Lifecycle, three-tier rebuild, correction, perf contracts, networks

[ewowi]

Summary

15-commit branch covering child-lifecycle propagation, the long-overdue closure of the coarse-rebuild debt, per-driver output correction, a real performance-contracts gate, NetworkModule polish, and the developer-environment Improv test loop.

Module lifecycle + module tree:

• MoonModule loop / loop20ms / loop1s base defaults now propagate to children via a shared tickChildren helper; the previously asymmetric "setup chains, loops don't" was historical, not principled. Override-and-chain convention codified in architecture.md § Lifecycle propagation to children.
• ImprovProvisioningModule moves to a child of NetworkModule (a four-line follow-up enabled by the propagation fix).
• markWiredByCode() flag on MoonModule preserves code-wired children across an applyNode load whose saved JSON predates the addition — fixes Improv vanishing on every reboot before this branch.

Control-change + rebuild:

• Three-tier handleSetControl: cheap onUpdate always, buildState() only when controlChangeTriggersBuildState(name) returns true (default false; overridden true on LayoutBase / ModifierBase). Slider drag no longer triggers a tree-wide realloc sweep. Mirrors MoonLight's onUpdate / requestMappings / onSizeChanged split.
• Renamed onAllocateMemory → onBuildState (idempotent, history-agnostic — boot and later changes are the same call).

Output correction:

• Per-driver stage shared via the Drivers container — Correction (256-entry brightness LUT + channel-order table + derive-white flag) handed to each physical driver via const Correction*. ArtNet was previously sending raw bytes; gap now closed. Preview exempt.

Performance contracts + telemetry:

• Per-target tick + heap + max_alloc_block contracts captured in every scenario JSON, with an observed.<target> widen-only sibling block tracking the rolling [min, max] across runs. run_scenario.py writes back observed values automatically.
• docs/performance.md now generated from the contracts — no hand-edited table to drift.
• ArtNet alloc moved off the hot path; platform clock seam kills the previously-flaky animation tests.

NetworkModule polish:

• New mode / rssi / txPower read-only metrics — required adding ControlType::ReadOnlyInt (int8 + unit suffix). Static-IP fields gain ControlType::IPv4 (4-byte storage instead of 16-char dotted-quad).
• Idle recovery: failed STA/Eth init now lands cleanly on Idle instead of crashing.

ControlType dispatcher:

• src/core/Control.cpp extracts the per-ControlType switch that had drifted across HttpServerModule::writeControls, FilesystemModule::writeValue, and scenario_runner.cpp. ApplyPolicy::Strict (HTTP) vs Clamp (FS load) preserves the tolerant-load semantics. JsonSink gains a fixed-buffer mode (third backend alongside socket + heap-grow) so the FS serializer shares the codepath without growing a per-value allocation. Codified as coding-standards.md § Per-type behaviour lives with the type.

MoonDeck:

• New "Networks" concept — bundles WiFi credentials + scanned devices + last-used serial port per network; auto-detects which network the laptop is currently on. Replaces the previous flat device list and per-config WiFi credentials file.
• Per-network state mutated under mutate_state(mutator) with RLock + atomic temp-file write.

Board → firmware rename:

• "Board" was overloaded between physical hardware and firmware variant. The firmware variant (esp32-eth-wifi etc.) is now firmware; board is reserved for physical hardware (a separate concept, see docs/plan.md § Board injection).

Test reorganization:

• test/unit/ split into core/ and light/ subdirectories with CamelCase naming matching the source module.
• Generated docs/tests/unit-tests.md + docs/tests/scenario-tests.md indices from source — no hand-edited test catalog to drift.
• scenario_runner and run_live_scenario now share contract semantics; min_pct bounds are runner-WARN, hardware-fail.

Developer-environment Improv test loop:

• scripts/run/preview_installer.py flash-ready mode stages every local build/esp32-*/projectMM.bin under releases/latest/ in the preview server's tree, generates Pages-relative manifests via the same script production uses, and overlays the GitHub latest tag — http://localhost:8000/ flashes the local build with full ESP Web Tools + Improv WiFi. Web Serial works on localhost without the secure-origin requirement.
• scripts/build/improv_smoke_test.py: three-step probe + WiFi provision + LAN reachability as a single CLI, exit codes for CI, wired into MoonDeck. Closes the gap that made every Improv-touching commit a high-friction "tag, wait for CI, deploy" cycle.

KPI (head of branch)

PC:356KB | tick:70/58/126/107us(FPS:14285/17241/7936/9345)
ESP32:1178KB | tick:83521us(FPS:11) | heap:154KB
src:60(9232) | test:33(4279) | lizard:33w

PC tick squarely in the historical 46–85us / 48–59us / 121–196us / 107–158us band. ESP32 tick from a live Olimex board.

Test plan

• Desktop build clean (zero warnings)
• ctest — all unit + scenario tests pass on every commit
• mm_scenarios (in-process) — all pass
• Live scenarios on Olimex ESP32-Gateway — run_live_scenario.py for the touched scenarios; observed bands within contract
• Platform boundary check — PASS
• Spec check — 25/25
• ESP32 build (esp32-eth-wifi) — clean
• Hardware verification on Olimex ESP32-Gateway:
  • Improv renders as a child of Network, survives reboot without erase_flash (the markWiredByCode fix)
  • Network mode / rssi / txPower metrics update live; mode lands Idle cleanly when init fails
  • Slider drag no longer triggers a tree-wide realloc sweep (three-tier control-change)
  • Output correction visible end-to-end via ArtNet (brightness scaling + channel reorder applied at the driver edge)
  • MoonDeck networks: laptop switches between two LANs, devices/wifi/port follow the active network
  • Local installer flash-ready mode: preview_installer.py → browser → flash → Improv WiFi → device reachable on LAN

Reviewer trail

• CodeRabbit findings processed across the branch — fixed in additional commits or skipped with a one-line reason on the PR thread.
• Opus Reviewer agent run at pre-merge over the full branch diff (git diff main...HEAD) — findings either fixed in commit 158d727 or accepted with reason.

🤖 Generated with Claude Code

[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: 1bb565a9-36f1-4bef-af49-ec4bcfd5092e

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

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@src/core/MoonModule.h`:
- Around line 41-43: Update the incorrect comment describing the enabled-gate
logic: replace the text that says "respectsEnabled() || enabled()" with the
actual gate used, "!respectsEnabled() || enabled()", in the comments around the
loop descriptions (e.g., the block mentioning loop20ms/loop1s) and the other
occurrence that documents the same contract; reference the actual functions
respectsEnabled() and enabled() so the comment matches the implemented
scheduler-equivalent logic.

In `@test/test_lifecycle_propagation.cpp`:
- Around line 136-143: The current assertion CHECK(a.loopTimeUs() >= 0u) is
tautological because loopTimeUs() is unsigned; make the test deterministic by
explicitly injecting non-zero accumulated microseconds before calling
publishTiming: call addAccumUs(1) (or a helper child loop that calls addAccumUs)
on the same object used in the test, then invoke publishTiming() and replace the
tautological check with CHECK(a.loopTimeUs() > 0u) to reliably catch
regressions; update the test helper or inline setup to ensure addAccumUs is
exercised (refer to loopTimeUs(), addAccumUs(), and publishTiming()).

🪄 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: 73f2933b-331c-483e-98a7-1af87591731a

📥 Commits

Reviewing files that changed from the base of the PR and between 6281ca5 and 9f46e27.

📒 Files selected for processing (15)

• CLAUDE.md
• docs/architecture.md
• docs/history/decisions.md
• docs/moonmodules/core/ImprovProvisioningModule.md
• docs/testing.md
• src/core/FilesystemModule.cpp
• src/core/MoonModule.h
• src/core/NetworkModule.h
• src/light/drivers/Drivers.h
• src/light/layers/Layers.h
• src/main.cpp
• src/ui/app.js
• test/CMakeLists.txt
• test/test_filesystem_persistence.cpp
• test/test_lifecycle_propagation.cpp

[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 (3)

src/core/FilesystemModule.cpp (1)

198-207: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Preserving a wired child here can still delete persisted siblings after it.

Breaking out when Line 207 hits a code-wired mismatch leaves jsonChildCount at the last matched slot, and the trim loop then removes every later non-wired child. A tree like [persisted A, wired X, persisted B] loaded from older JSON [A, B] will keep X but delete B on that boot.

Please reconcile JSON indices against the next non-wired live child instead of aborting the whole tail, or otherwise skip trimming past the first preserved wired mismatch.

Also applies to: 243-252

🤖 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.cpp` around lines 198 - 207, The current
early-break when encountering a code-wired child (live && live->isWiredByCode())
preserves that single wired node but leaves jsonChildCount at the last matched
index so the trim loop deletes later persisted siblings; instead, when you
detect a wired mismatch in the reconciliation loop (the block that checks live =
m->child(i), strcmp(live->typeName(), typeName) != 0, and
live->isWiredByCode()), scan forward from that live index to find the next
non-wired live child (or end) and advance the JSON index (jsonChildCount / the
index variable used for trimming) to the position that aligns with that next
non-wired live child, then break so the trim loop will only remove nodes after
that aligned index; apply the identical logic to the second reconciliation
region noted (lines 243-252) so trimming always reconciles against the next
non-wired live child rather than aborting the whole tail.

src/core/HttpServerModule.cpp (1)

527-545: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject writes to immutable controls before the common update path.

ReadOnly and Progress currently fall through into onUpdate(), markDirty(), and a 200 {"ok":true} response even though no value changed. That lets a bad client trigger side effects or needless persistence/build-state work on controls that are supposed to be immutable.

💡 Minimal fix

             case ControlType::ReadOnly:
             case ControlType::Progress:
-                break; // read-only, skip
+                sendResponse(conn, 400, "application/json", "{\"error\":\"control is read-only\"}");
+                return;
         }

🤖 Prompt for AI Agents

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

In `@src/core/HttpServerModule.cpp` around lines 527 - 545, The code currently
lets ControlType::ReadOnly and ControlType::Progress fall through into the
common update path (target->onUpdate, target->markDirty,
FilesystemModule::noteDirty, scheduler_->buildState) even for immutable
controls; change the control handling so that when c.type ==
ControlType::ReadOnly or ControlType::Progress the handler returns early with an
error/reject response (do not call target->onUpdate, target->markDirty,
FilesystemModule::noteDirty, or scheduler_->buildState) so writes are rejected
before any side effects or persistence work occur.

src/core/NetworkModule.h (1)

34-41: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Schedule persistence when credentials come from Improv.

This path only sets the module-local dirty bit. Unlike the HTTP control path, it never notifies the persistence debounce, so WiFi credentials pushed through setWifiCredentials() can be lost on reboot unless some unrelated later change happens to call the global dirty trigger. Please route this through the same dirty-notification path as other persisted control mutations.

🤖 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/NetworkModule.h` around lines 34 - 41, setWifiCredentials currently
only sets the module-local dirty bit via markDirty(), so Improv-sourced
credential changes can be lost; after copying ssid_ and password_ and calling
markDirty(), also invoke the same global persistence/debounce notifier used by
other control paths (i.e. the global persistence debounce function called by
HTTP control mutations) so FilesystemModule picks up the change
immediately—locate setWifiCredentials and replace or augment the markDirty()
call with a call to the shared persistence-notification routine used elsewhere
in the codebase.

🤖 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/decisions.md`:
- Around line 579-582: Update the decision doc to use the new lifecycle API
names: replace references to Scheduler::rebuild() with buildState(),
onAllocateMemory() with onBuildState(), and controlChangeNeedsRebuild(...) with
controlChangeTriggersBuildState(...). Also update the sentence describing
defaults/overrides to say controlChangeTriggersBuildState defaults to false and
is overridden to true on LayoutBase and ModifierBase, and ensure any mentions of
MoonModule::onUpdate or the three-tier behavior reference the new
buildState/onBuildState split where appropriate (use the exact symbols
buildState(), onBuildState(), controlChangeTriggersBuildState(...) in the text).

In `@docs/moonmodules/core/MoonModule.md`:
- Around line 67-68: Update the lifecycle propagation documentation to include
loop20ms() and loop1s() alongside the other propagated methods: modify the
paragraph that lists propagated hooks (currently naming setup(),
onBuildControls(), onBuildState(), loop(), and teardown()) to also mention
loop20ms() and loop1s() so it accurately reflects that parents forward setup(),
onBuildControls(), onBuildState(), loop(), loop20ms(), loop1s(), and teardown()
to their children (reference the method symbols loop20ms and loop1s to locate
the change).

In `@src/light/drivers/ArtNetSendDriver.h`:
- Around line 62-87: The hot path in loop() currently calls
corrected_.allocate(...) (via corrected_.count()/channelsPerLight() checks)
which can perform heap allocation; move that sizing logic out of the render hot
path and into the event handlers that change topology/preset/correction (e.g.
wherever correction_ is set or when sourceBuffer_ topology changes). Ensure that
when correction_->outChannels or sourceBuffer_->count()/channelsPerLight()
changes you precompute and call corrected_.allocate(newCount, newChannels) (and
validate corrected_.data()) so loop() only reads corrected_.data()/count() and
never allocates; keep the existing fallback to passthrough if allocation failed
but ensure allocations happen off the hot path.

In `@src/light/drivers/Correction.h`:
- Around line 39-55: rebuild() currently assumes the casted LightPreset is
always valid; add validation at the start of the function (or handle via a
switch default) to clamp/normalize any out-of-range preset to a safe default
(e.g. LightPreset::RGB) before executing the existing switch so that order[],
outChannels and deriveWhite are always set deterministically; specifically, in
rebuild(uint8_t brightness, LightPreset preset) check the incoming preset value
against the known enum range and if invalid set preset = LightPreset::RGB (or
route to the existing RGB case), or add a switch default that explicitly assigns
order[0]=0, order[1]=1, order[2]=2, outChannels=3, deriveWhite=false to
guarantee safe behavior.

In `@src/light/layers/Layer.h`:
- Around line 65-66: In onBuildState (Layer::onBuildState) avoid returning early
when layouts_ is null; instead reset the layer to the same clean state you use
for the physicalCount == 0 path: clear or reset the LUT/container that holds the
lookup table, release or null out the buffer, reset dimensions
(width/height/stride) and any size counters (e.g., dynamicBytes/dynamic_bytes
accounting), and reset any flags that indicate a built state so the layer is
truly empty when layouts_ is not wired. Locate the onBuildState method and apply
the same cleanup logic used below for the physicalCount == 0 branch to the case
where layouts_ == nullptr.

In `@test/test_phase_animation.cpp`:
- Around line 40-41: The test currently probes animation by sleeping real time
(e.g., via delayMs()) after calling layouts.onBuildState() and
layer.onBuildState(), which makes it timing-dependent; replace that wall-clock
probing with a fake/test clock seam: hook the animation timing into the test
clock (or platform test hook) and, instead of calling delayMs(), advance the
test clock by the desired milliseconds and drive the animation tick/update (so
any timers/animation frames use the fake clock). Update the test to call the
platform/test-clock advance method (or inject a FakeClock) around
layouts.onBuildState() and layer.onBuildState(), and ensure the animation update
path reads the test clock so assertions run deterministically.

---

Outside diff comments:
In `@src/core/FilesystemModule.cpp`:
- Around line 198-207: The current early-break when encountering a code-wired
child (live && live->isWiredByCode()) preserves that single wired node but
leaves jsonChildCount at the last matched index so the trim loop deletes later
persisted siblings; instead, when you detect a wired mismatch in the
reconciliation loop (the block that checks live = m->child(i),
strcmp(live->typeName(), typeName) != 0, and live->isWiredByCode()), scan
forward from that live index to find the next non-wired live child (or end) and
advance the JSON index (jsonChildCount / the index variable used for trimming)
to the position that aligns with that next non-wired live child, then break so
the trim loop will only remove nodes after that aligned index; apply the
identical logic to the second reconciliation region noted (lines 243-252) so
trimming always reconciles against the next non-wired live child rather than
aborting the whole tail.

In `@src/core/HttpServerModule.cpp`:
- Around line 527-545: The code currently lets ControlType::ReadOnly and
ControlType::Progress fall through into the common update path
(target->onUpdate, target->markDirty, FilesystemModule::noteDirty,
scheduler_->buildState) even for immutable controls; change the control handling
so that when c.type == ControlType::ReadOnly or ControlType::Progress the
handler returns early with an error/reject response (do not call
target->onUpdate, target->markDirty, FilesystemModule::noteDirty, or
scheduler_->buildState) so writes are rejected before any side effects or
persistence work occur.

In `@src/core/NetworkModule.h`:
- Around line 34-41: setWifiCredentials currently only sets the module-local
dirty bit via markDirty(), so Improv-sourced credential changes can be lost;
after copying ssid_ and password_ and calling markDirty(), also invoke the same
global persistence/debounce notifier used by other control paths (i.e. the
global persistence debounce function called by HTTP control mutations) so
FilesystemModule picks up the change immediately—locate setWifiCredentials and
replace or augment the markDirty() call with a call to the shared
persistence-notification routine used elsewhere in the codebase.

🪄 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: c9d77b40-2fd3-4b3b-93a3-9254a42c667c

📥 Commits

Reviewing files that changed from the base of the PR and between 9f46e27 and aaf8d98.

📒 Files selected for processing (49)

• docs/architecture.md
• docs/history/decisions.md
• docs/moonmodules/core/FilesystemModule.md
• docs/moonmodules/core/HttpServerModule.md
• docs/moonmodules/core/MoonModule.md
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/core/ui.md
• docs/moonmodules/light/Drivers.md
• docs/moonmodules/light/Layouts.md
• docs/moonmodules/light/drivers/PreviewDriver.md
• docs/moonmodules/light/effects/FireEffect.md
• docs/moonmodules/light/effects/ParticlesEffect.md
• docs/moonmodules/light/effects/PlasmaEffect.md
• docs/plan.md
• docs/testing.md
• src/core/FilesystemModule.cpp
• src/core/FilesystemModule.h
• src/core/HttpServerModule.cpp
• src/core/MoonModule.h
• src/core/NetworkModule.h
• src/core/Scheduler.cpp
• src/core/Scheduler.h
• src/light/drivers/ArtNetSendDriver.h
• src/light/drivers/Correction.h
• src/light/drivers/Drivers.h
• src/light/drivers/PreviewDriver.h
• src/light/effects/FireEffect.h
• src/light/effects/ParticlesEffect.h
• src/light/layers/Layer.h
• src/light/layers/Layers.h
• src/light/layouts/Layouts.h
• src/light/modifiers/ModifierBase.h
• test/CMakeLists.txt
• test/test_correction.cpp
• test/test_effects_zero_grid.cpp
• test/test_extrude.cpp
• test/test_fire.cpp
• test/test_layers_container.cpp
• test/test_layout_toggle_cycle.cpp
• test/test_lifecycle_propagation.cpp
• test/test_metaballs.cpp
• test/test_noise.cpp
• test/test_particles.cpp
• test/test_phase_animation.cpp
• test/test_plasma.cpp
• test/test_preview_driver.cpp
• test/test_rainbow.cpp
• test/test_replacechild.cpp
• test/test_stateless_effects.cpp

[coderabbitai]

Actionable comments posted: 13

Caution

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

⚠️ Outside diff range comments (12)

test/unit/core/unit_MoonModule.cpp (1)

17-18: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

This test bypasses the MoonModule lifecycle code it claims to cover.

TestModule::setup() and teardown() replace the base implementation entirely, so TEST_CASE("MoonModule lifecycle") only proves the stub booleans flip. It will stay green even if mm::MoonModule regresses its new default lifecycle behavior. Call mm::MoonModule::setup()/teardown() from the overrides, or better, override hook methods and test the real entry points.

One minimal fix

 class TestModule : public mm::MoonModule {
 public:
@@
-    void setup() override { setupCalled = true; }
-    void teardown() override { teardownCalled = true; }
+    void setup() override {
+        mm::MoonModule::setup();
+        setupCalled = true;
+    }
+    void teardown() override {
+        mm::MoonModule::teardown();
+        teardownCalled = true;
+    }

Based on learnings, new core logic needs a corresponding module test.

Also applies to: 29-37

🤖 Prompt for AI Agents

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

In `@test/unit/core/unit_MoonModule.cpp` around lines 17 - 18, The test overrides
TestModule::setup() and teardown() and so bypasses mm::MoonModule lifecycle
logic; modify TestModule so its overrides call the base implementations (e.g.,
mm::MoonModule::setup() and mm::MoonModule::teardown()) or instead override
specific protected hook methods that mm::MoonModule invokes, then update
TEST_CASE("MoonModule lifecycle") to assert behavior through the real entry
points (setup()/teardown()) rather than only checking local stub booleans.
Ensure references to TestModule::setup, TestModule::teardown,
mm::MoonModule::setup, mm::MoonModule::teardown and the TEST_CASE are used to
locate and change the code.

test/unit/core/unit_MappingLUT.cpp (1)

79-82: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Verify the actual destinations for logical index 3.

This branch only checks the count. If forEachDestination(3, ...) returns the wrong three indices or the wrong order, the test still passes. Assert {6, 8, 9} explicitly like the earlier cases.

🤖 Prompt for AI Agents

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

In `@test/unit/core/unit_MappingLUT.cpp` around lines 79 - 82, The test currently
only checks the count from lut.forEachDestination(3, ...) but not the actual
values; update the test to collect destinations into dests (as already done) and
assert that dests equals the expected sequence {6, 8, 9} (including order)
instead of only checking size — use the same comparison style as earlier cases
to verify exact contents for logical index 3.

test/unit/core/unit_Buffer.cpp (1)

68-71: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert that failed allocate() leaves the buffer empty.

Right now this only checks the return value. A regression that returns false after partially changing data(), count(), or channelsPerLight() would still pass. Add postconditions for the unchanged/empty state.

🤖 Prompt for AI Agents

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

In `@test/unit/core/unit_Buffer.cpp` around lines 68 - 71, The test must also
assert that a failed mm::Buffer::allocate(...) leaves the buffer in an
empty/unchanged state; update TEST_CASE "Buffer allocate with zero returns
false" to, after each CHECK_FALSE(buf.allocate(...)), add checks that buf.data()
is null (or equivalent empty pointer), buf.count() == 0, and
buf.channelsPerLight() == 0 to ensure no partial modifications occurred by
allocate().

test/unit/core/unit_FilesystemModule_persistence.cpp (2)

291-296: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parse the file instead of using string heuristics.

This test claims “valid JSON”, but the assertions only check for a couple of commas and the absence of "". A malformed document can still satisfy those checks. Feed content into the same JSON parser used by the module and assert parse success.

🤖 Prompt for AI Agents

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

In `@test/unit/core/unit_FilesystemModule_persistence.cpp` around lines 291 - 296,
Replace the fragile string-heuristic checks on the variable content (loaded from
".config/Layer.json") with a real JSON parse using the same parser the module
uses (e.g., call the module's parse function or
nlohmann::json::parse/Json::Reader on content), assert the parse succeeds (no
exception/false result), then assert the parsed document contains the expected
fields/values (e.g., that the appropriate array/object contains "MirrorModifier"
and "NoiseEffect") instead of checking for substrings or the absence of "".

---

24-88: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard fsSetRoot() and teardown with RAII.

These tests mutate global filesystem state and only restore it at the end of the function. Any earlier REQUIRE abort leaves the wrong root installed for later tests, so one failure can cascade into unrelated failures across the suite. Wrap the root swap and scheduler teardown in a local guard instead of open-coded cleanup.

Possible pattern

+struct FsRootGuard {
+    explicit FsRootGuard(const char* root) { mm::platform::fsSetRoot(root); }
+    ~FsRootGuard() { mm::platform::fsSetRoot("."); }
+};
+
 TEST_CASE("FilesystemModule round-trip") {
     char tmpRoot[256];
     std::snprintf(tmpRoot, sizeof(tmpRoot), "/tmp/mm_persist_test_%u",
                   static_cast<unsigned>(mm::platform::millis()));
     std::filesystem::remove_all(tmpRoot);
-    mm::platform::fsSetRoot(tmpRoot);
+    FsRootGuard rootGuard(tmpRoot);
     ...
-    std::filesystem::remove_all(tmpRoot);
-    mm::platform::fsSetRoot("."); // restore default
+    std::filesystem::remove_all(tmpRoot);
 }

🤖 Prompt for AI Agents

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

In `@test/unit/core/unit_FilesystemModule_persistence.cpp` around lines 24 - 88,
Wrap the global root swap and scheduler teardown in small RAII guards: add a
RootGuard that saves the current root, calls mm::platform::fsSetRoot(tmpRoot) in
the constructor and in its destructor calls std::filesystem::remove_all(tmpRoot)
and mm::platform::fsSetRoot(oldRoot); and add a SchedulerGuard (or similar) that
takes a mm::Scheduler* and calls scheduler->teardown() in its destructor; then
use RootGuard at the start of the TEST_CASE and create a SchedulerGuard
immediately after each mm::Scheduler scheduler; this ensures
mm::platform::fsSetRoot and Scheduler::teardown are always restored even on
early test aborts.

test/unit/core/unit_MoonModule_lifecycle.cpp (1)

123-150: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

This test doesn't actually pin child-vs-parent timing attribution.

Line 140 injects the timing directly into a, so the test still passes if tickChildren() stops attributing measured child time entirely. It also never asserts that the parent stayed at 0, so the “not the parent” part of the contract is untested. Please make this case fail when attribution moves off the child path. Based on learnings: new core logic needs a corresponding module test.

🤖 Prompt for AI Agents

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

In `@test/unit/core/unit_MoonModule_lifecycle.cpp` around lines 123 - 150, The
test currently injects timing into the child directly (a.addAccumUs(40)), so it
won't catch regressions where tickChildren stops attributing measured time to
children; change the injection so the timing is added to the parent and relies
on tickChildren to forward it to the child: call parent.addAccumUs(40) before
parent.loop()/publishTiming(2) (or add it then call parent.loop()), then assert
that a.loopTimeUs() >= 20u and that parent.loopTimeUs() remains 0u to verify the
attribution went to the child (references: Counting, parent.addChild,
parent.addAccumUs, parent.loop, parent.publishTiming, a.loopTimeUs).

test/unit/light/unit_Correction.cpp (1)

117-127: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Pin the invalid-LightPreset fallback too.

This suite covers the valid presets, but it never exercises the new default branch in Correction::rebuild(). Add one case that casts an out-of-range enum value and verifies outChannels, deriveWhite, and channel order fall back to the safe preset instead of preserving stale state.

Based on learnings "Tests must pass: run ctest (unit tests) and ./build/test/mm_scenarios (scenarios) before considering work complete; new core logic needs a corresponding module test; full pipeline needs a scenario test".

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_Correction.cpp` around lines 117 - 127, Add a new unit
test that forces the invalid-presets branch by calling Correction::rebuild with
a casted out-of-range LightPreset value (e.g. static_cast<LightPreset>(999)) and
then assert that Correction::outChannels, Correction::deriveWhite, and the
channel ordering (the same member used elsewhere to represent RGB order) match
the safe fallback preset (the default used in rebuild) instead of preserving
previous state; place it alongside the existing "rebuild switches output channel
count RGB<->RGBW" test so it exercises the fallback path and ensures rebuild
resets all related members when given an invalid LightPreset.

test/unit/light/unit_FireEffect.cpp (1)

77-79: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Exercise the parent onBuildState() path here.

This assertion currently bypasses the lifecycle propagation that the PR is changing. If Layer::onBuildState() stops forwarding rebuilds to disabled effects, this test still passes because it calls fire.onBuildState() directly. Rebuild through layer instead.

Proposed fix

     fire.setEnabled(false);
-    fire.onBuildState();
+    layer.onBuildState();
     CHECK(fire.dynamicBytes() == 0);

Based on learnings "Tests must pass: run ctest (unit tests) and ./build/test/mm_scenarios (scenarios) before considering work complete; new core logic needs a corresponding module test; full pipeline needs a scenario test".

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_FireEffect.cpp` around lines 77 - 79, The test currently
calls fire.onBuildState() directly which bypasses Layer::onBuildState()
lifecycle propagation; modify the test to disable the effect via
fire.setEnabled(false), then trigger the rebuild through the parent layer (call
layer.onBuildState() or whatever public API the test uses to rebuild the layer)
and then assert CHECK(fire.dynamicBytes() == 0) so the assertion exercises
Layer::onBuildState() forwarding behavior for disabled effects; locate
references to fire, layer, Layer::onBuildState(), and dynamicBytes() in
unit_FireEffect.cpp to update the test accordingly.

test/unit/light/unit_ParticlesEffect.cpp (1)

54-75: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Rebuild the owning Layer for the disable case.

Calling particles.onBuildState() directly bypasses the parent lifecycle sequencing that production uses after control changes. This can hide integration bugs in the layer-managed teardown path.

Suggested fix

     layer.onBuildState();
     CHECK(particles.dynamicBytes() > 0);

     particles.setEnabled(false);
-    particles.onBuildState();
+    layer.onBuildState();
     CHECK(particles.dynamicBytes() == 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 `@test/unit/light/unit_ParticlesEffect.cpp` around lines 54 - 75, The test
currently calls particles.onBuildState() after particles.setEnabled(false),
which bypasses the parent lifecycle; instead, invoke the owning Layer's
lifecycle so the Layer-managed teardown runs: after calling
particles.setEnabled(false) call layer.onBuildState() (and remove the direct
particles.onBuildState() call) so the ParticlesEffect teardown is exercised via
Layer.

test/unit/light/unit_Layouts_toggle_cycle.cpp (1)

85-97: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert the driver saw an empty source on the disabled tick.

The regression note says the driver should see an empty source after the only layout is disabled, but this block only checks Layouts/Layer state. A stale forwarded buffer would still pass here.

Suggested assertion additions

     grid.setEnabled(false);
     rebuildAndTick(layouts, layersC, driversC);
     CHECK(layouts.totalLightCount() == 0);
     CHECK(layer.physicalLightCount() == 0);
     CHECK_FALSE(layer.lut().hasLUT());  // LUT torn down, not stale
+    CHECK(capture.lastBytes == 0);
+    CHECK_FALSE(capture.lastNonZero);

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_Layouts_toggle_cycle.cpp` around lines 85 - 97, After
disabling the only layout (grid.setEnabled(false)) you must assert the driver
actually saw an empty source before re-enabling: after calling
rebuildAndTick(layouts, layersC, driversC) add checks on the driver capture
(e.g., CHECK_MESSAGE(capture.lastBytes == 0, ...) and
CHECK_MESSAGE(!capture.lastNonZero, ...)) to ensure no stale forwarded buffer
was sent; keep the existing layout/layer assertions and then proceed with the
re-enable and its existing driver checks.

test/unit/light/unit_NoiseEffect.cpp (1)

46-70: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add precondition asserts before the raw buffer reads.

These tests dereference layer.buffer().data() directly. If onBuildState() ever stops allocating the buffer, they will segfault instead of reporting a clean test failure.

At minimum, add REQUIRE(data != nullptr) before the corner reads, and keep the z-slice tests gated by a size check before hashing into the slices.

Also applies to: 115-163

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_NoiseEffect.cpp` around lines 46 - 70, Add null-pointer
and size precondition checks before reading raw buffer data in this test: after
calling layer.onBuildState() and before dereferencing layer.buffer().data(),
REQUIRE that data != nullptr; additionally guard any multi-slice or z-indexed
accesses (the later z-slice hashing checks referenced around lines 115-163) with
a size check on layer.buffer().size() (or equivalent) to ensure the buffer is
large enough before computing indexes; this prevents segfaults if onBuildState()
fails to allocate and keeps checks local to the NoiseEffect test using layer,
layer.onBuildState(), layer.buffer().data(), and layer.buffer().size().

scripts/moondeck_ui/app.js (1)

413-451: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Honor Stop across multi-device live runs.

Clicking Stop here only kills the currently active child process. The outer for (const device of devices) loop keeps iterating, so live_scenario continues on the remaining selected devices even though the UI says the run was stopped. Track a per-script cancel flag/abort state and break the loop before scheduling the next host.

🤖 Prompt for AI Agents

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

In `@scripts/moondeck_ui/app.js` around lines 413 - 451, The multi-device live-run
loop in runScript currently continues iterating after a Stop because it only
kills the child process but doesn't signal the outer loop to abort; modify
runScript so the devices loop respects an abort/cancel state (e.g., a per-script
flag or AbortController) and break before scheduling the next host when
cancellation is detected. Concretely, keep or set a cancel token when Stop is
requested (the same spot that calls fetch("/api/kill/" + script.id)), pass/check
that token into/after each runScriptOnce call (or have runScriptOnce return a
clearly-defined aborted/failed result), and if the token indicates cancellation
(or runScriptOnce returns aborted), break the for (const device of devices) loop
and return so remaining devices are not processed.

♻️ Duplicate comments (2)

test/unit/light/unit_Layer_phase_animation.cpp (1)

30-56: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

These animation tests are still wall-clock dependent.

Line 50 and Line 108 still sleep real time to prove animation, so the suite remains timing-dependent and susceptible to CI load and timer-resolution variance. Push elapsed time through a fake/test clock seam and advance that clock in the assertions instead of calling delayMs().

As per coding guidelines "test/**: Unit and integration tests using doctest. Verify tests cover edge cases and match the specifications in docs/moonmodules/. Tests should not depend on timing or network."

Also applies to: 83-112

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_Layer_phase_animation.cpp` around lines 30 - 56, The
test currently depends on wall-clock sleeps via mm::platform::delayMs(step_ms)
inside animates_over_ms; replace that real-time wait with a test-controlled
clock seam: make the timing source injectable (e.g., add a Clock/ITimer
interface or allow passing a Clock to Layer/Effect or mm::platform) and in the
unit test instantiate a Fake/TestClock, advance it by step_ms between loop()
calls (call testClock->advanceMs(step_ms) or equivalent) instead of calling
mm::platform::delayMs, then call layer.loop() and compare buffers; update
animates_over_ms to use the fake clock so tests are deterministic and CI-safe.

test/unit/light/unit_CheckerboardEffect.cpp (1)

122-140: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Replace the wall-clock probe with a test-time seam.

Lines 135-139 still use mm::platform::delayMs(50) to make the effect advance, so this test remains timing-dependent and can go flaky across CI load or platform timer behavior. Drive the animation from a fake/test clock instead of sleeping real time.

As per coding guidelines "test/**: Unit and integration tests using doctest. Verify tests cover edge cases and match the specifications in docs/moonmodules/. Tests should not depend on timing or network."

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_CheckerboardEffect.cpp` around lines 122 - 140, Replace
the real sleep with a simulated/test clock drive: remove the
mm::platform::delayMs(50) call and instead advance the test clock and/or the
effect's time explicitly between iterations (e.g. use a Ctx test helper like
advanceTimeMs(...) or call an effect test API such as advanceMs(...) or
setPhase()/setTime() before invoking ctx.layer.loop()). Specifically, in the
loop that calls ctx.layer.loop() for mm::LavaLampEffect, increment the fake
clock by ~50ms each iteration and call ctx.layer.loop() so the animation
progresses deterministically without wall-clock sleeps.

🤖 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/coding-standards.md`:
- Around line 25-41: Add explicit chaining guidance for onBuildControls() and
onBuildState() in the MoonModule lifecycle rules: state that onBuildControls()
should chain (call MoonModule::onBuildControls()) before performing parent work
so children register controls first, and onBuildState() should chain before
parent work so children contribute state before the parent reads or modifies it;
reference the MoonModule class and the existing patterns for loop(), setup(),
and teardown to mirror phrasing and include the same allowance for exceptions
(permit children-first only with a one-line comment explaining the reason).

In `@docs/moonmodules/light/EffectBase.md`:
- Line 39: Update the docs reference in EffectBase.md: the test filename
currently written as `unit_layer_extrude.cpp` is incorrect—replace it with the
actual test filename `unit_Layer_extrude.cpp` wherever mentioned (line
referencing "Unit tests: Layer" / `unit_layer_extrude.cpp`) so the docs match
the test inventory and filename convention.

In `@docs/moonmodules/light/effects/GlowParticlesEffect.md`:
- Line 25: Update the wording that incorrectly labels GlowParticlesEffect as a
"stateless effect": find the mention of "GlowParticlesEffect" alongside the link
to "CheckerboardEffect" and replace the phrase "stateless effects" with "covered
by shared baseline effect assertions" (or an equivalent phrase) so the
documentation reflects that GlowParticlesEffect maintains persistent
per-particle state rather than being stateless.

In `@docs/moonmodules/light/effects/LavaLampEffect.md`:
- Line 22: Update the test-note sentence that currently calls LavaLampEffect
“stateless”: change the wording to indicate that LavaLampEffect is covered by
the same unit test section as CheckerboardEffect without asserting statelessness
(referencing the doc phrase "LavaLampEffect" and the test link "[Unit tests:
CheckerboardEffect]"). Replace the claim about statelessness with something like
“LavaLampEffect is included in the shared test coverage — non-zero output,
spatial variation,” ensuring the note only states shared coverage and does not
describe LavaLampEffect’s state behavior.

In `@docs/moonmodules/light/effects/RipplesEffect.md`:
- Line 25: Update the sentence referring to "RipplesEffect" to remove the
incorrect claim that it is a stateless effect: edit the line in RipplesEffect.md
to either (a) point to the correct Ripples-specific test coverage that documents
its random respawn and per-ripple mutable state (position, radius, hue) or (b)
reword the link to the CheckerboardEffect test as a shared rendering/smoke test
(e.g., "shared rendering/smoke test (non-zero output, spatial variation)") so it
no longer asserts RipplesEffect is stateless; reference the RipplesEffect symbol
when making the change so the intent is explicit.

In `@docs/testing.md`:
- Around line 36-46: The fenced code blocks in docs/testing.md (the
directory-tree examples starting with "test/" and "scripts/docs/") are missing
language identifiers; update those triple-backtick fences to include a language
tag (e.g., ```text) so the blocks comply with MD040 and render correctly—locate
the two blocks that show the tree listings (one beginning with "test/" and the
other beginning with "scripts/docs/") and add the same language tag to both
opening fences.

In `@scripts/moondeck.py`:
- Around line 434-460: The handler _serve_doc currently serves .md files but
does not rewrite relative markdown links to the server endpoint, breaking repo
portability; modify either _serve_doc (before calling _serve_markdown_as_html)
or preferably inside _serve_markdown_as_html to post-process the rendered or raw
markdown and rewrite relative .md links to point to /api/docs/<path> (preserving
subpaths and anchors) while leaving absolute URLs and root-relative links alone
and not rewriting image links. Implement a regex replacement that targets
markdown links like [text](relative/path.md#anchor) but ignores links starting
with http(s):// or / and ignores ![image](...), converting them to
[text](/api/docs/relative/path.md#anchor); ensure the rewrite uses the same
filename/path resolution logic (md_path / docs root) so links remain valid for
served docs.

In `@scripts/scenario/run_scenario.py`:
- Around line 50-51: Add the explicit non-throwing contract to the
subprocess.run calls by passing check=False to each invocation (the three places
where subprocess.run is called, e.g., the call using RUNNER and scenario_file in
run_scenario.py). Locate the subprocess.run calls (references: subprocess.run,
RUNNER, scenario_file, and any other similar invocations in this script) and add
the keyword argument check=False (preserving existing cwd or other kwargs) so
the code relies on returncode instead of exceptions.

In `@src/light/drivers/Correction.h`:
- Around line 54-57: Clamp the raw preset value into range using
kLightPresetCount (e.g., if preset is an integer index, set preset =
std::min(preset, kLightPresetCount-1)) before the switch on the LightPreset enum
so any corrupt/out-of-range persisted value is mapped deterministically, then
remove the default: arm from the switch in Correction.h; keep the existing
RGB-handling logic in the case for LightPreset::RGB to initialize order[],
outChannels and deriveWhite so apply() never reads stale fields and new enum
values become compile-visible if unhandled.

In `@test/scenarios/core/scenario_MoonModule_control_change.json`:
- Line 5: Update the "description" field in
scenario_MoonModule_control_change.json to replace the stale reference
"unit-tested in Correction" with the correct companion reference (e.g., "covered
by the MoonModule control-change gate tests" or similar wording). Locate the
description string (the JSON "description" key referencing MirrorModifier's
mirrorX/Y) and edit its text so generated test docs point to the correct
module/gate tests.

In `@test/scenarios/core/scenario_NetworkModule_mdns_toggle.json`:
- Around line 4-32: This automated scenario relies on live network timing and
FPS thresholds and should be changed: either mark the scenario as
manual/hardware-only (so steps "baseline-mdns-on", "mdns-off", "mdns-on-again"
are gated) or remove the flaky timing assertion by replacing the "bounds": {
"fps": { "min_pct": 80 } } check with a deterministic assertion that verifies
the Network control "mDNS" was toggled to the expected boolean values (check the
"value": true/false for those steps) rather than measuring runtime FPS or
external mDNS traffic.

In `@test/scenarios/light/scenario_MirrorModifier_memory_lut.json`:
- Line 5: Update the companion scenario reference inside the JSON "description"
field: replace the old identifier "memory-1to1" with the new fixture name
"scenario_Layer_memory_1to1" so the cross-reference is correct and the generated
test inventory is not broken.

In `@test/unit/light/unit_Layers_container.cpp`:
- Around line 20-23: The test currently allows frame drift via
platform::millis() and only asserts non-empty outputs; replace any
time-dependent or random effect with a deterministic one (e.g., a static color
fill or a fixed-seed procedural pattern) so the bare Layer and the one-child
Layers container render identical bytes; specifically, remove or mock out
platform::millis() usage, construct the same deterministic Layer for both paths
(the reference Layer and the Layer wrapped by mm::Layouts / Layers container
used in this TEST_CASE "Layers with one Layer produces the same output as a bare
Layer"), render both to buffers and assert a strict byte-for-byte equality of
the output buffers.

---

Outside diff comments:
In `@scripts/moondeck_ui/app.js`:
- Around line 413-451: The multi-device live-run loop in runScript currently
continues iterating after a Stop because it only kills the child process but
doesn't signal the outer loop to abort; modify runScript so the devices loop
respects an abort/cancel state (e.g., a per-script flag or AbortController) and
break before scheduling the next host when cancellation is detected. Concretely,
keep or set a cancel token when Stop is requested (the same spot that calls
fetch("/api/kill/" + script.id)), pass/check that token into/after each
runScriptOnce call (or have runScriptOnce return a clearly-defined
aborted/failed result), and if the token indicates cancellation (or
runScriptOnce returns aborted), break the for (const device of devices) loop and
return so remaining devices are not processed.

In `@test/unit/core/unit_Buffer.cpp`:
- Around line 68-71: The test must also assert that a failed
mm::Buffer::allocate(...) leaves the buffer in an empty/unchanged state; update
TEST_CASE "Buffer allocate with zero returns false" to, after each
CHECK_FALSE(buf.allocate(...)), add checks that buf.data() is null (or
equivalent empty pointer), buf.count() == 0, and buf.channelsPerLight() == 0 to
ensure no partial modifications occurred by allocate().

In `@test/unit/core/unit_FilesystemModule_persistence.cpp`:
- Around line 291-296: Replace the fragile string-heuristic checks on the
variable content (loaded from ".config/Layer.json") with a real JSON parse using
the same parser the module uses (e.g., call the module's parse function or
nlohmann::json::parse/Json::Reader on content), assert the parse succeeds (no
exception/false result), then assert the parsed document contains the expected
fields/values (e.g., that the appropriate array/object contains "MirrorModifier"
and "NoiseEffect") instead of checking for substrings or the absence of "".
- Around line 24-88: Wrap the global root swap and scheduler teardown in small
RAII guards: add a RootGuard that saves the current root, calls
mm::platform::fsSetRoot(tmpRoot) in the constructor and in its destructor calls
std::filesystem::remove_all(tmpRoot) and mm::platform::fsSetRoot(oldRoot); and
add a SchedulerGuard (or similar) that takes a mm::Scheduler* and calls
scheduler->teardown() in its destructor; then use RootGuard at the start of the
TEST_CASE and create a SchedulerGuard immediately after each mm::Scheduler
scheduler; this ensures mm::platform::fsSetRoot and Scheduler::teardown are
always restored even on early test aborts.

In `@test/unit/core/unit_MappingLUT.cpp`:
- Around line 79-82: The test currently only checks the count from
lut.forEachDestination(3, ...) but not the actual values; update the test to
collect destinations into dests (as already done) and assert that dests equals
the expected sequence {6, 8, 9} (including order) instead of only checking size
— use the same comparison style as earlier cases to verify exact contents for
logical index 3.

In `@test/unit/core/unit_MoonModule_lifecycle.cpp`:
- Around line 123-150: The test currently injects timing into the child directly
(a.addAccumUs(40)), so it won't catch regressions where tickChildren stops
attributing measured time to children; change the injection so the timing is
added to the parent and relies on tickChildren to forward it to the child: call
parent.addAccumUs(40) before parent.loop()/publishTiming(2) (or add it then call
parent.loop()), then assert that a.loopTimeUs() >= 20u and that
parent.loopTimeUs() remains 0u to verify the attribution went to the child
(references: Counting, parent.addChild, parent.addAccumUs, parent.loop,
parent.publishTiming, a.loopTimeUs).

In `@test/unit/core/unit_MoonModule.cpp`:
- Around line 17-18: The test overrides TestModule::setup() and teardown() and
so bypasses mm::MoonModule lifecycle logic; modify TestModule so its overrides
call the base implementations (e.g., mm::MoonModule::setup() and
mm::MoonModule::teardown()) or instead override specific protected hook methods
that mm::MoonModule invokes, then update TEST_CASE("MoonModule lifecycle") to
assert behavior through the real entry points (setup()/teardown()) rather than
only checking local stub booleans. Ensure references to TestModule::setup,
TestModule::teardown, mm::MoonModule::setup, mm::MoonModule::teardown and the
TEST_CASE are used to locate and change the code.

In `@test/unit/light/unit_Correction.cpp`:
- Around line 117-127: Add a new unit test that forces the invalid-presets
branch by calling Correction::rebuild with a casted out-of-range LightPreset
value (e.g. static_cast<LightPreset>(999)) and then assert that
Correction::outChannels, Correction::deriveWhite, and the channel ordering (the
same member used elsewhere to represent RGB order) match the safe fallback
preset (the default used in rebuild) instead of preserving previous state; place
it alongside the existing "rebuild switches output channel count RGB<->RGBW"
test so it exercises the fallback path and ensures rebuild resets all related
members when given an invalid LightPreset.

In `@test/unit/light/unit_FireEffect.cpp`:
- Around line 77-79: The test currently calls fire.onBuildState() directly which
bypasses Layer::onBuildState() lifecycle propagation; modify the test to disable
the effect via fire.setEnabled(false), then trigger the rebuild through the
parent layer (call layer.onBuildState() or whatever public API the test uses to
rebuild the layer) and then assert CHECK(fire.dynamicBytes() == 0) so the
assertion exercises Layer::onBuildState() forwarding behavior for disabled
effects; locate references to fire, layer, Layer::onBuildState(), and
dynamicBytes() in unit_FireEffect.cpp to update the test accordingly.

In `@test/unit/light/unit_Layouts_toggle_cycle.cpp`:
- Around line 85-97: After disabling the only layout (grid.setEnabled(false))
you must assert the driver actually saw an empty source before re-enabling:
after calling rebuildAndTick(layouts, layersC, driversC) add checks on the
driver capture (e.g., CHECK_MESSAGE(capture.lastBytes == 0, ...) and
CHECK_MESSAGE(!capture.lastNonZero, ...)) to ensure no stale forwarded buffer
was sent; keep the existing layout/layer assertions and then proceed with the
re-enable and its existing driver checks.

In `@test/unit/light/unit_NoiseEffect.cpp`:
- Around line 46-70: Add null-pointer and size precondition checks before
reading raw buffer data in this test: after calling layer.onBuildState() and
before dereferencing layer.buffer().data(), REQUIRE that data != nullptr;
additionally guard any multi-slice or z-indexed accesses (the later z-slice
hashing checks referenced around lines 115-163) with a size check on
layer.buffer().size() (or equivalent) to ensure the buffer is large enough
before computing indexes; this prevents segfaults if onBuildState() fails to
allocate and keeps checks local to the NoiseEffect test using layer,
layer.onBuildState(), layer.buffer().data(), and layer.buffer().size().

In `@test/unit/light/unit_ParticlesEffect.cpp`:
- Around line 54-75: The test currently calls particles.onBuildState() after
particles.setEnabled(false), which bypasses the parent lifecycle; instead,
invoke the owning Layer's lifecycle so the Layer-managed teardown runs: after
calling particles.setEnabled(false) call layer.onBuildState() (and remove the
direct particles.onBuildState() call) so the ParticlesEffect teardown is
exercised via Layer.

---

Duplicate comments:
In `@test/unit/light/unit_CheckerboardEffect.cpp`:
- Around line 122-140: Replace the real sleep with a simulated/test clock drive:
remove the mm::platform::delayMs(50) call and instead advance the test clock
and/or the effect's time explicitly between iterations (e.g. use a Ctx test
helper like advanceTimeMs(...) or call an effect test API such as advanceMs(...)
or setPhase()/setTime() before invoking ctx.layer.loop()). Specifically, in the
loop that calls ctx.layer.loop() for mm::LavaLampEffect, increment the fake
clock by ~50ms each iteration and call ctx.layer.loop() so the animation
progresses deterministically without wall-clock sleeps.

In `@test/unit/light/unit_Layer_phase_animation.cpp`:
- Around line 30-56: The test currently depends on wall-clock sleeps via
mm::platform::delayMs(step_ms) inside animates_over_ms; replace that real-time
wait with a test-controlled clock seam: make the timing source injectable (e.g.,
add a Clock/ITimer interface or allow passing a Clock to Layer/Effect or
mm::platform) and in the unit test instantiate a Fake/TestClock, advance it by
step_ms between loop() calls (call testClock->advanceMs(step_ms) or equivalent)
instead of calling mm::platform::delayMs, then call layer.loop() and compare
buffers; update animates_over_ms to use the fake clock so tests are
deterministic and CI-safe.

🪄 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: 2f78e70e-a547-439c-aa62-50ef974170aa

📥 Commits

Reviewing files that changed from the base of the PR and between aaf8d98 and 25d5db3.

📒 Files selected for processing (90)

• CLAUDE.md
• docs/architecture.md
• docs/coding-standards.md
• docs/history/decisions.md
• docs/moonmodules/core/Control.md
• docs/moonmodules/core/MoonModule.md
• docs/moonmodules/light/Buffer.md
• docs/moonmodules/light/EffectBase.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/LightConfig.md
• docs/moonmodules/light/drivers/ArtNetSendDriver.md
• docs/moonmodules/light/drivers/PreviewDriver.md
• docs/moonmodules/light/effects/CheckerboardEffect.md
• docs/moonmodules/light/effects/FireEffect.md
• docs/moonmodules/light/effects/GlowParticlesEffect.md
• docs/moonmodules/light/effects/LavaLampEffect.md
• docs/moonmodules/light/effects/MetaballsEffect.md
• docs/moonmodules/light/effects/NoiseEffect.md
• docs/moonmodules/light/effects/ParticlesEffect.md
• docs/moonmodules/light/effects/PlasmaEffect.md
• docs/moonmodules/light/effects/PlasmaPaletteEffect.md
• docs/moonmodules/light/effects/RainbowEffect.md
• docs/moonmodules/light/effects/RipplesEffect.md
• docs/moonmodules/light/effects/SpiralEffect.md
• docs/moonmodules/light/layouts/GridLayout.md
• docs/moonmodules/light/leddriver-analysis-bottom-up.md
• docs/moonmodules/light/leddriver-analysis-top-down.md
• docs/moonmodules/light/modifiers/MirrorModifier.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• scripts/MoonDeck.md
• scripts/docs/_test_metadata.py
• scripts/docs/generate_test_docs.py
• scripts/moondeck.py
• scripts/moondeck_config.json
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/style.css
• scripts/scenario/run_live_scenario.py
• scripts/scenario/run_scenario.py
• scripts/test/test_desktop.py
• src/core/HttpServerModule.cpp
• src/core/NetworkModule.h
• src/light/drivers/Correction.h
• 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/grid-resize.json
• test/scenarios/light/scenario_GridLayout_resize.json
• test/scenarios/light/scenario_Layer_base_pipeline.json
• test/scenarios/light/scenario_Layer_memory_1to1.json
• test/scenarios/light/scenario_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/scenarios/light/scenario_PreviewDriver_detail.json
• test/scenarios/mdns-toggle.json
• test/unit/core/unit_Buffer.cpp
• test/unit/core/unit_Color.cpp
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_ImprovFrame.cpp
• test/unit/core/unit_MappingLUT.cpp
• test/unit/core/unit_ModuleFactory.cpp
• test/unit/core/unit_MoonModule.cpp
• test/unit/core/unit_MoonModule_control_change_gate.cpp
• test/unit/core/unit_MoonModule_lifecycle.cpp
• test/unit/core/unit_MoonModule_movechild.cpp
• test/unit/core/unit_MoonModule_replacechild.cpp
• test/unit/core/unit_NetworkModule.cpp
• test/unit/core/unit_Scheduler_unique_names.cpp
• test/unit/core/unit_SystemModule.cpp
• test/unit/light/unit_ArtNetSendDriver_packet.cpp
• test/unit/light/unit_BlendMap.cpp
• test/unit/light/unit_CheckerboardEffect.cpp
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_Drivers_container.cpp
• test/unit/light/unit_FireEffect.cpp
• test/unit/light/unit_GridLayout.cpp
• test/unit/light/unit_Layer_extrude.cpp
• test/unit/light/unit_Layer_phase_animation.cpp
• test/unit/light/unit_Layer_zero_grid.cpp
• test/unit/light/unit_Layers_container.cpp
• test/unit/light/unit_Layouts_container.cpp
• test/unit/light/unit_Layouts_toggle_cycle.cpp
• test/unit/light/unit_MetaballsEffect.cpp
• test/unit/light/unit_MirrorModifier.cpp
• test/unit/light/unit_NoiseEffect.cpp
• test/unit/light/unit_ParticlesEffect.cpp
• test/unit/light/unit_PlasmaEffect.cpp
• test/unit/light/unit_PreviewDriver.cpp
• test/unit/light/unit_RainbowEffect.cpp

💤 Files with no reviewable changes (2)

• test/scenarios/grid-resize.json
• test/scenarios/mdns-toggle.json

[ewowi]

@coderabbitai, can you review 4bddecd and compare against open conversations as those were addressed in this commit, please close open conversations when applicable

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

REPO_OWNER="ewowi"
REPO_NAME="projectMM"
PR_NUMBER="9"
COMMIT_SHA="4bddecd207e7e29d0a708100b556baa6426a8b5f"

echo "=== PR basic info ==="
gh pr view "$PR_NUMBER" --repo "$REPO_OWNER/$REPO_NAME" --json number,title,state,headRefName,baseRefName,headRefOid,url

echo
echo "=== Commit summary ==="
git show --no-ext-diff --stat --oneline --decorate --find-renames "$COMMIT_SHA" || true

echo
echo "=== Commit changed files ==="
git show --no-ext-diff --name-status --find-renames --format="" "$COMMIT_SHA" || true

echo
echo "=== Unresolved review threads (all authors, compact) ==="
PR_NODE_ID="$(gh pr view "$PR_NUMBER" --repo "$REPO_OWNER/$REPO_NAME" --json id --jq .id)"
gh api graphql -f query='
query($id: ID!) {
  node(id: $id) {
    ... on PullRequest {
      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first: 20) {
            nodes {
              id
              databaseId
              author { login }
              body
              url
              path
              line
              originalLine
              commit { oid }
              createdAt
            }
          }
        }
      }
    }
  }
}' -f id="$PR_NODE_ID" \
| jq -r '
  .data.node.reviewThreads.nodes[]
  | select(.isResolved == false)
  | {
      threadId: .id,
      isOutdated: .isOutdated,
      path: .path,
      line: .line,
      originalLine: .originalLine,
      firstAuthor: (.comments.nodes[0].author.login // null),
      firstCommentId: (.comments.nodes[0].id // null),
      firstDatabaseId: (.comments.nodes[0].databaseId // null),
      firstUrl: (.comments.nodes[0].url // null),
      firstCommit: (.comments.nodes[0].commit.oid // null),
      firstBodyPreview: ((.comments.nodes[0].body // "") | gsub("\r";"") | gsub("\n";" ") | .[0:500]),
      replies: [
        .comments.nodes[]
        | {
            author: (.author.login // null),
            databaseId,
            url,
            bodyPreview: ((.body // "") | gsub("\r";"") | gsub("\n";" ") | .[0:300])
          }
      ]
    }
' | jq -s '.'

echo
echo "=== Search specified commit diff for likely review-fix markers ==="
git show --no-ext-diff --find-renames --format=fuller "$COMMIT_SHA" -- \
  ':(exclude)**/*.png' ':(exclude)**/*.jpg' ':(exclude)**/*.jpeg' ':(exclude)**/*.gif' \
  | sed -n '1,260p'

Length of output: 29989

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

COMMIT_SHA="4bddecd207e7e29d0a708100b556baa6426a8b5f"
REPO_OWNER="ewowi"
REPO_NAME="projectMM"
PR_NUMBER="9"

echo "=== Full diff for key implementation files ==="
git show --no-ext-diff --find-renames --format=medium "$COMMIT_SHA" -- \
  src/light/layers/Layer.h \
  scripts/moondeck.py \
  scripts/scenario/run_scenario.py \
  test/unit/light/unit_Layouts_toggle_cycle.cpp \
  test/unit/core/unit_Buffer.cpp \
  test/unit/core/unit_MappingLUT.cpp

echo
echo "=== Current snippets: scripts/moondeck.py markdown serving/link rewrite ==="
python - <<'PY'
from pathlib import Path
p = Path("scripts/moondeck.py")
lines = p.read_text().splitlines()
for start,end in [(650,780), (780,860)]:
    print(f"\n--- {p}:{start}-{end} ---")
    for i,line in enumerate(lines[start-1:end], start):
        print(f"{i:4}: {line}")
PY

echo
echo "=== Current snippets: src/light/layers/Layer.h onBuildState ==="
python - <<'PY'
from pathlib import Path
p = Path("src/light/layers/Layer.h")
lines = p.read_text().splitlines()
for start,end in [(1,180)]:
    print(f"\n--- {p}:{start}-{end} ---")
    for i,line in enumerate(lines[start-1:end], start):
        print(f"{i:4}: {line}")
PY

echo
echo "=== Full bodies of unresolved CodeRabbit threads ==="
PR_NODE_ID="$(gh pr view "$PR_NUMBER" --repo "$REPO_OWNER/$REPO_NAME" --json id --jq .id)"
gh api graphql -f query='
query($id: ID!) {
  node(id: $id) {
    ... on PullRequest {
      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first: 20) {
            nodes {
              id
              databaseId
              author { login }
              body
              url
              createdAt
            }
          }
        }
      }
    }
  }
}' -f id="$PR_NODE_ID" \
| jq -r '
  .data.node.reviewThreads.nodes[]
  | select(.isResolved == false)
  | select(.comments.nodes[0].author.login == "coderabbitai")
  | "THREAD " + .id + " path=" + (.path // "") + " line=" + ((.line // .originalLine // 0)|tostring) + " outdated=" + (.isOutdated|tostring) + "\nURL " + (.comments.nodes[0].url // "") + "\nBODY:\n" + (.comments.nodes[0].body // "") + "\n---"
'

Length of output: 49003

---

---

@ewowi I reviewed commit 4bddecd207e7e29d0a708100b556baa6426a8b5f against the currently open CodeRabbit conversations.

Summary:

• ✅ The new Layer::onBuildState() null-layout behavior looks correct: layouts_ == nullptr now uses the same empty-layout teardown path, clearing stale LUT/buffer/dimensions/status/dynamic bytes.
• ✅ The unit_Layouts_toggle_cycle regression assertions now cover the stale-buffer path more directly.
• ✅ The MoonDeck relative .md link rewrite looks consistent with the /api/docs/... rendering flow.
• ✅ The run_scenario.py check=False changes are behavior-preserving and make the return-code contract explicit.
• ✅ The Buffer and MappingLUT test tightenings look fine.

I did not close any currently open conversations, because the unresolved threads are still not fixed by this commit; the commit message explicitly marks them as deferred/kept as-is:

• ArtNetSendDriver.h allocation-in-loop() — deferred.
• Correction.h enum switch/default exhaustiveness nit — deferred.
• scenario_NetworkModule_mdns_toggle.json timing/network scenario concern — kept as-is.
• unit_Layers_container.cpp byte-exact deterministic comparison — deferred.

No new blocker from this focused commit review.

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

REPO="ewowi/projectMM"
PR_NUMBER="9"
COMMIT_SHA="4bddecd207e7e29d0a708100b556baa6426a8b5f"

echo "== PR metadata =="
gh pr view "$PR_NUMBER" --repo "$REPO" --json number,title,headRefName,baseRefName,state,url,reviewDecision,latestReviews

echo
echo "== Target commit metadata/files =="
gh api "repos/$REPO/commits/$COMMIT_SHA" \
  --jq '{sha:.sha, message:.commit.message, author:.commit.author, files:[.files[] | {filename, status, additions, deletions, changes, patch:(.patch // "")[0:1200]}]}'

echo
echo "== Unresolved review threads =="
PR_NODE_ID="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json id --jq .id)"

gh api graphql -f query='
query($id: ID!) {
  node(id: $id) {
    ... on PullRequest {
      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          startLine
          originalStartLine
          diffSide
          comments(first: 20) {
            nodes {
              id
              databaseId
              author { login }
              body
              createdAt
              outdated
              path
              line
              originalLine
              url
              diffHunk
              pullRequestReview { id databaseId state author { login } }
            }
          }
        }
      }
    }
  }
}' -f id="$PR_NODE_ID" --jq '
.node.reviewThreads.nodes
| map(select(.isResolved == false))
| .[]
| {
    threadId: .id,
    isOutdated: .isOutdated,
    path: .path,
    line: .line,
    originalLine: .originalLine,
    comments: [.comments.nodes[] | {
      author: .author.login,
      body: .body,
      createdAt: .createdAt,
      outdated: .outdated,
      path: .path,
      line: .line,
      originalLine: .originalLine,
      url: .url,
      diffHunk: .diffHunk
    }]
  }'

Length of output: 70577

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

REPO="ewowi/projectMM"
PR_NUMBER="9"
COMMIT_SHA="4bddecd207e7e29d0a708100b556baa6426a8b5f"

echo "== PR metadata =="
gh pr view "$PR_NUMBER" --repo "$REPO" --json number,title,headRefName,baseRefName,state,url,reviewDecision,latestReviews

echo
echo "== Target commit metadata/files =="
gh api "repos/$REPO/commits/$COMMIT_SHA" \
  --jq '{sha:.sha, message:.commit.message, author:.commit.author, files:[.files[] | {filename, status, additions, deletions, changes, patch:(.patch // "")[0:1200]}]}'

echo
echo "== Unresolved review threads =="
PR_NODE_ID="$(gh pr view "$PR_NUMBER" --repo "$REPO" --json id --jq .id)"

gh api graphql -f query='
query($id: ID!) {
  node(id: $id) {
    ... on PullRequest {
      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          startLine
          originalStartLine
          diffSide
          comments(first: 20) {
            nodes {
              id
              databaseId
              author { login }
              body
              createdAt
              outdated
              path
              line
              originalLine
              url
              diffHunk
              pullRequestReview { id databaseId state author { login } }
            }
          }
        }
      }
    }
  }
}' -f id="$PR_NODE_ID" --jq '
.node.reviewThreads.nodes
| map(select(.isResolved == false))
| .[]
| {
    threadId: .id,
    isOutdated: .isOutdated,
    path: .path,
    line: .line,
    originalLine: .originalLine,
    comments: [.comments.nodes[] | {
      author: .author.login,
      body: .body,
      createdAt: .createdAt,
      outdated: .outdated,
      path: .path,
      line: .line,
      originalLine: .originalLine,
      url: .url,
      diffHunk: .diffHunk
    }]
  }'

Length of output: 70577

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER="ewowi"
NAME="projectMM"
PR_NUMBER=9

echo "== Unresolved review threads via repository.pullRequest =="
gh api graphql -f query='
query($owner: String!, $name: String!, $number: Int!) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first: 20) {
            nodes {
              id
              author { login }
              body
              createdAt
              outdated
              path
              line
              originalLine
              url
              diffHunk
            }
          }
        }
      }
    }
  }
}' -f owner="$OWNER" -f name="$NAME" -F number="$PR_NUMBER" --jq '
.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false))
| .[]
| {
    threadId: .id,
    isOutdated: .isOutdated,
    path: .path,
    line: .line,
    originalLine: .originalLine,
    firstAuthor: (.comments.nodes[0].author.login // null),
    firstBody: ((.comments.nodes[0].body // "") | split("\n")[0:4] | join(" ")),
    comments: [.comments.nodes[] | {
      author: .author.login,
      bodyFirstLines: (.body | split("\n")[0:5] | join(" ")),
      url: .url,
      path: .path,
      line: .line,
      originalLine: .originalLine
    }]
  }'

Length of output: 1754

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER="ewowi"
NAME="projectMM"
PR_NUMBER=9

echo "== Unresolved review threads via repository.pullRequest =="
gh api graphql -f query='
query($owner: String!, $name: String!, $number: Int!) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      reviewThreads(first: 100) {
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first: 20) {
            nodes {
              id
              author { login }
              body
              createdAt
              outdated
              path
              line
              originalLine
              url
              diffHunk
            }
          }
        }
      }
    }
  }
}' -f owner="$OWNER" -f name="$NAME" -F number="$PR_NUMBER" --jq '
.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false))
| .[]
| {
    threadId: .id,
    isOutdated: .isOutdated,
    path: .path,
    line: .line,
    originalLine: .originalLine,
    firstAuthor: (.comments.nodes[0].author.login // null),
    firstBody: ((.comments.nodes[0].body // "") | split("\n")[0:4] | join(" ")),
    comments: [.comments.nodes[] | {
      author: .author.login,
      bodyFirstLines: (.body | split("\n")[0:5] | join(" ")),
      url: .url,
      path: .path,
      line: .line,
      originalLine: .originalLine
    }]
  }'

Length of output: 1754

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER="ewowi"
NAME="projectMM"
REPO="$OWNER/$NAME"
PR_NUMBER=9

echo "== Raw GraphQL response for thread query =="
gh api graphql -f query='
query($owner: String!, $name: String!, $number: Int!) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      id
      number
      reviewThreads(first: 20) {
        nodes {
          id
          isResolved
          path
          comments(first: 5) {
            nodes {
              author { login }
              body
              url
            }
          }
        }
      }
    }
  }
}' -f owner="$OWNER" -f name="$NAME" -F number="$PR_NUMBER" || true

echo
echo "== REST review comments on PR (filtered to likely addressed files) =="
gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" \
  --jq '
  map(select(.user.login == "coderabbitai"))
  | map(select(.path as $p | [
      "docs/coding-standards.md",
      "docs/moonmodules/light/EffectBase.md",
      "docs/moonmodules/light/Layer.md",
      "docs/moonmodules/light/effects/GlowParticlesEffect.md",
      "docs/moonmodules/light/effects/LavaLampEffect.md",
      "docs/moonmodules/light/effects/RipplesEffect.md",
      "docs/testing.md",
      "scripts/moondeck.py",
      "scripts/scenario/run_scenario.py",
      "test/scenarios/core/scenario_MoonModule_control_change.json",
      "test/scenarios/light/scenario_MirrorModifier_memory_lut.json",
      "test/unit/core/unit_Buffer.cpp",
      "test/unit/core/unit_MappingLUT.cpp",
      "test/unit/light/unit_Layouts_toggle_cycle.cpp"
    ] | index($p)))
  | .[]
  | {
      id,
      path,
      line,
      original_line,
      outdated,
      bodyFirstLines: (.body | split("\n")[0:6] | join(" ")),
      html_url
    }'

Length of output: 51940

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER="ewowi"
NAME="projectMM"
REPO="$OWNER/$NAME"
PR_NUMBER=9

echo "== Raw GraphQL response for thread query =="
gh api graphql -f query='
query($owner: String!, $name: String!, $number: Int!) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      id
      number
      reviewThreads(first: 20) {
        nodes {
          id
          isResolved
          path
          comments(first: 5) {
            nodes {
              author { login }
              body
              url
            }
          }
        }
      }
    }
  }
}' -f owner="$OWNER" -f name="$NAME" -F number="$PR_NUMBER" || true

echo
echo "== REST review comments on PR (filtered to likely addressed files) =="
gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" \
  --jq '
  map(select(.user.login == "coderabbitai"))
  | map(select(.path as $p | [
      "docs/coding-standards.md",
      "docs/moonmodules/light/EffectBase.md",
      "docs/moonmodules/light/Layer.md",
      "docs/moonmodules/light/effects/GlowParticlesEffect.md",
      "docs/moonmodules/light/effects/LavaLampEffect.md",
      "docs/moonmodules/light/effects/RipplesEffect.md",
      "docs/testing.md",
      "scripts/moondeck.py",
      "scripts/scenario/run_scenario.py",
      "test/scenarios/core/scenario_MoonModule_control_change.json",
      "test/scenarios/light/scenario_MirrorModifier_memory_lut.json",
      "test/unit/core/unit_Buffer.cpp",
      "test/unit/core/unit_MappingLUT.cpp",
      "test/unit/light/unit_Layouts_toggle_cycle.cpp"
    ] | index($p)))
  | .[]
  | {
      id,
      path,
      line,
      original_line,
      outdated,
      bodyFirstLines: (.body | split("\n")[0:6] | join(" ")),
      html_url
    }'

Length of output: 51940

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER="ewowi"
NAME="projectMM"
PR_NUMBER=9

python3 - <<'PY'
import json
import subprocess

owner = "ewowi"
name = "projectMM"
number = "9"

query = r'''
query($owner: String!, $name: String!, $number: Int!, $after: String) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      reviewThreads(first: 100, after: $after) {
        pageInfo { hasNextPage endCursor }
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first: 10) {
            nodes {
              author { login }
              body
              url
            }
          }
        }
      }
    }
  }
}
'''

after = None
all_threads = []
while True:
    cmd = [
        "gh", "api", "graphql",
        "-f", f"query={query}",
        "-f", f"owner={owner}",
        "-f", f"name={name}",
        "-F", f"number={number}",
    ]
    if after:
        cmd.extend(["-f", f"after={after}"])
    raw = subprocess.check_output(cmd, text=True)
    data = json.loads(raw)
    if data.get("errors"):
        print("GraphQL errors:", json.dumps(data["errors"], indent=2))
        raise SystemExit(1)
    threads = data["data"]["repository"]["pullRequest"]["reviewThreads"]
    all_threads.extend(threads["nodes"])
    if not threads["pageInfo"]["hasNextPage"]:
        break
    after = threads["pageInfo"]["endCursor"]

print(f"Total review threads: {len(all_threads)}")
print("== Unresolved threads ==")
for t in all_threads:
    if t["isResolved"]:
        continue
    first = t["comments"]["nodes"][0] if t["comments"]["nodes"] else {}
    body = (first.get("body") or "").splitlines()
    title = next((line for line in body if line.startswith("**") and line.endswith("**")), body[0] if body else "")
    print(json.dumps({
        "threadId": t["id"],
        "path": t["path"],
        "line": t["line"],
        "originalLine": t["originalLine"],
        "firstAuthor": (first.get("author") or {}).get("login"),
        "titleOrFirstLine": title,
        "url": first.get("url"),
    }, ensure_ascii=False))
PY

Length of output: 3497

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

OWNER="ewowi"
NAME="projectMM"
PR_NUMBER=9

python3 - <<'PY'
import json
import subprocess

owner = "ewowi"
name = "projectMM"
number = "9"

query = r'''
query($owner: String!, $name: String!, $number: Int!, $after: String) {
  repository(owner: $owner, name: $name) {
    pullRequest(number: $number) {
      reviewThreads(first: 100, after: $after) {
        pageInfo { hasNextPage endCursor }
        nodes {
          id
          isResolved
          isOutdated
          path
          line
          originalLine
          comments(first: 10) {
            nodes {
              author { login }
              body
              url
            }
          }
        }
      }
    }
  }
}
'''

after = None
all_threads = []
while True:
    cmd = [
        "gh", "api", "graphql",
        "-f", f"query={query}",
        "-f", f"owner={owner}",
        "-f", f"name={name}",
        "-F", f"number={number}",
    ]
    if after:
        cmd.extend(["-f", f"after={after}"])
    raw = subprocess.check_output(cmd, text=True)
    data = json.loads(raw)
    if data.get("errors"):
        print("GraphQL errors:", json.dumps(data["errors"], indent=2))
        raise SystemExit(1)
    threads = data["data"]["repository"]["pullRequest"]["reviewThreads"]
    all_threads.extend(threads["nodes"])
    if not threads["pageInfo"]["hasNextPage"]:
        break
    after = threads["pageInfo"]["endCursor"]

print(f"Total review threads: {len(all_threads)}")
print("== Unresolved threads ==")
for t in all_threads:
    if t["isResolved"]:
        continue
    first = t["comments"]["nodes"][0] if t["comments"]["nodes"] else {}
    body = (first.get("body") or "").splitlines()
    title = next((line for line in body if line.startswith("**") and line.endswith("**")), body[0] if body else "")
    print(json.dumps({
        "threadId": t["id"],
        "path": t["path"],
        "line": t["line"],
        "originalLine": t["originalLine"],
        "firstAuthor": (first.get("author") or {}).get("login"),
        "titleOrFirstLine": title,
        "url": first.get("url"),
    }, ensure_ascii=False))
PY

Length of output: 3497

---

---

@ewowi I reviewed commit 4bddecd207e7e29d0a708100b556baa6426a8b5f against the PR’s review threads.

Result: no additional conversations needed closing. The findings clearly addressed by that commit were already marked resolved, including:

• src/light/layers/Layer.h — null layouts_ stale-state reset
• docs/coding-standards.md — onBuildControls() / onBuildState() chaining guidance
• docs/moonmodules/light/EffectBase.md and related effect docs — stale filename/stateless wording fixes
• docs/testing.md — fenced code block language tags
• scripts/moondeck.py — relative .md link rewrite for MoonDeck docs
• scripts/scenario/run_scenario.py — explicit check=False
• scenario description cross-reference fixes
• several generated/test documentation follow-ups

There are 4 CodeRabbit threads still open, and I left them open because this commit does not address them or explicitly defers/keeps the current behavior:

1. src/light/drivers/ArtNetSendDriver.h — move corrected_ sizing/allocation out of loop()
2. src/light/drivers/Correction.h — keep LightPreset switch exhaustive while handling invalid persisted values
3. test/scenarios/core/scenario_NetworkModule_mdns_toggle.json — automated network/timing scenario concern
4. test/unit/light/unit_Layers_container.cpp — deterministic single-child equivalence check

So the currently applicable commit-addressed conversations are already closed; the remaining open ones still represent unresolved/deferred review items.

[coderabbitai]

Actionable comments posted: 13

Caution

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

⚠️ Outside diff range comments (10)

src/light/layers/Layer.h (1)

80-87: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reset lutSkipped_ in the empty-layer branch.

This branch clears the visible status, but lutSkipped_ keeps its previous value because rebuildLUT() is skipped. After a prior low-memory degrade, an empty layer can still report lutSkipped()==true with no LUT left.

🐛 Minimal fix

         if (physicalCount == 0) {
+            lutSkipped_ = false;
             physicalWidth_ = physicalHeight_ = physicalDepth_ = 0;
             width_ = height_ = depth_ = 0;
             lut_.free();
             buffer_.free();

🤖 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 80 - 87, The empty-layer branch that
runs when physicalCount == 0 clears widths, frees lut_ and buffer_, and clears
status but does not reset lutSkipped_, so a previously degraded state can still
report lutSkipped()==true; update the branch (near physicalCount == 0 handling
where physicalWidth_/physicalHeight_/physicalDepth_, width_/height_/depth_,
lut_.free(), buffer_.free(), setDynamicBytes(0), clearStatus(),
MoonModule::onBuildState() are called) to explicitly set lutSkipped_ = false so
rebuildLUT() being skipped won't leave stale lutSkipped_ state.

src/light/drivers/Drivers.h (1)

102-103: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add the exception-reason comment for this parent-first onBuildState().

This override deliberately reverses the new default ordering so child drivers enter onBuildState() with setSourceBuffer() / setCorrection() already wired. Please say that directly at the MoonModule::onBuildState() call so a later cleanup does not “fix” the order back.

♻️ Minimal doc-only fix

         setDynamicBytes(outputBuffer_.bytes());
         passBufferToDrivers();
+        // Parent-first onBuildState: child drivers need source/correction wiring before their own onBuildState().
         MoonModule::onBuildState();

As per coding guidelines, **/*.{cpp,h,hpp}: “Use exception-reason comments when deviating from established patterns in the codebase.” Based on learnings, bespoke patterns should be justified with “a one-line comment at the introduction site.”

🤖 Prompt for AI Agents

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

In `@src/light/drivers/Drivers.h` around lines 102 - 103, Add a one-line
exception-reason comment immediately above the MoonModule::onBuildState() call
to document that this override intentionally invokes the parent first (after
passBufferToDrivers()) so child drivers run their onBuildState() with
setSourceBuffer() and setCorrection() already wired; mention explicitly that
this reverses the new default ordering on purpose to prevent future maintainers
from “fixing” the order back.

scripts/docs/_test_metadata.py (1)

114-123: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Include @also / also modules in the shared module index.

paths_for_module() and cases_for_module() already treat secondary modules as filterable, but list_test_modules() drops them here. Any module that is only referenced through @also / also will never appear in MoonDeck's module dropdown, so its tests stay undiscoverable even though the lookup layer supports them.

Suggested fix

 def list_test_modules() -> list[str]:
     """Union of `@module` values across unit tests + scenarios, sorted."""
     modules: set[str] = set()
     for f in collect_unit_files():
         if f["module"]:
             modules.add(f["module"])
+        modules.update(f["also"])
     for s in collect_scenario_files():
         if s["module"]:
             modules.add(s["module"])
+        modules.update(s["also"])
     return sorted(modules)

🤖 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/docs/_test_metadata.py` around lines 114 - 123, list_test_modules()
only collects primary "module" values from collect_unit_files() and
collect_scenario_files(), so modules referenced only via the secondary
"`@also`"/"also" field are omitted; update list_test_modules to also extract and
add any entries from the secondary modules field (e.g., f.get("also") /
s.get("also") or whatever key the file dict uses for `@also`) into the modules set
when iterating over collect_unit_files() and collect_scenario_files(),
normalizing them to strings before adding and then return sorted(modules).

scripts/moondeck.py (1)

657-682: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restrict /api/doc-asset to documentation assets.

This handler will currently serve any file under the repository root, and main() exposes MoonDeck on all interfaces. That means a LAN client can fetch repo-local files like scripts/moondeck.json, source files, or other non-doc artifacts via /api/doc-asset/.... Please narrow this to the directories and file types the markdown renderer actually needs.

🤖 Prompt for AI Agents

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

In `@scripts/moondeck.py` around lines 657 - 682, The _serve_doc_asset handler
currently serves any file under ROOT; restrict it to only renderer-needed doc
asset directories and safe file types: add a whitelist of allowed subdirectories
(e.g., "docs", "site", "assets" or whatever your markdown renderer uses) and
ensure asset_path.relative_to(ROOT / allowed_dir) succeeds for one of those
allowed dirs, and also enforce a whitelist of allowed extensions/mime types
(images, css, js) before reading/writing; additionally update main() so the
server binds to localhost (127.0.0.1) or make the bind host configurable instead
of listening on all interfaces to prevent LAN access. Ensure you change checks
in _serve_doc_asset, reference ROOT and asset_path, and update main()'s server
bind call accordingly.

test/scenario_runner.cpp (1)

281-303: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

PreviewDriver fixture steps never receive a frame.

The PreviewDriver branch is nested under if (step.has("props")), but the new fix-preview fixture in test/scenarios/light/scenario_PreviewDriver_detail.json has no props. That means setPreviewFrame() is never called for the exact scenario this PR adds.

Suggested patch

         if (step.has("props")) {
             auto& props = step["props"];
             if (std::strcmp(type, "Layer") == 0) {
                 auto* layer = static_cast<mm::Layer*>(mod);
                 if (props.has("layouts")) {
                     auto* layoutsModule = static_cast<mm::Layouts*>(modules[props["layouts"].str]);
                     if (layoutsModule) layer->setLayouts(layoutsModule);
                 }
                 if (props.has("channelsPerLight")) {
                     layer->setChannelsPerLight(static_cast<uint8_t>(props["channelsPerLight"].num));
                 }
             } else if (std::strcmp(type, "Drivers") == 0) {
                 if (props.has("layer")) {
                     auto* layerModule = static_cast<mm::Layer*>(modules[props["layer"].str]);
                     if (layerModule) static_cast<mm::Drivers*>(mod)->setLayer(layerModule);
                 }
-            } else if (std::strcmp(type, "PreviewDriver") == 0) {
-                // PreviewDriver writes into a PreviewFrame that HttpServerModule
-                // reads in production. The scenario runner has no HttpServer; we
-                // give the driver a process-static frame so its loop() has a
-                // valid write target and tick measurements are honest.
-                static_cast<mm::PreviewDriver*>(mod)->setPreviewFrame(&scenarioPreviewFrame());
             }
         }
+        if (std::strcmp(type, "PreviewDriver") == 0) {
+            // PreviewDriver writes into a PreviewFrame that HttpServerModule
+            // reads in production. The scenario runner has no HttpServer; we
+            // give the driver a process-static frame so its loop() has a
+            // valid write target and tick measurements are honest.
+            static_cast<mm::PreviewDriver*>(mod)->setPreviewFrame(&scenarioPreviewFrame());
+        }

🤖 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 281 - 303, The PreviewDriver handling
is incorrectly nested inside the if (step.has("props")) block so
setPreviewFrame() is never called for fixtures that lack props; move or
duplicate the PreviewDriver branch so that when std::strcmp(type,
"PreviewDriver") == 0 you always call
static_cast<mm::PreviewDriver*>(mod)->setPreviewFrame(&scenarioPreviewFrame())
regardless of step.has("props"), e.g., extract the PreviewDriver branch out of
the props conditional (or add a separate check after it) so setPreviewFrame and
scenarioPreviewFrame are always invoked for PreviewDriver modules.

test/scenarios/light/scenario_MirrorModifier_pipeline.json (1)

64-91: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the mirror pipeline scenario functional, not performance-gated.

Using ArtNet plus bounded FPS here makes the scenario depend on network and timing behavior instead of only validating the mirror pipeline wiring. Keep the end-to-end construction/assertion flow in this file, and move the throughput contract to a dedicated benchmark path.

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

🤖 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/light/scenario_MirrorModifier_pipeline.json` around lines 64 -
91, The scenario "add-artnet" (id "ArtNet", type "ArtNetSendDriver") currently
enables measurement and a bounded FPS contract (fields measure, bounds,
contract, observed) which makes the test depend on network/timing; remove or
disable those measurement/contract fields from this scenario so it only
validates mirror pipeline wiring (keep the module addition and end-to-end
construction/assertion flow intact), and move the throughput contract and
observed metrics into a separate benchmark scenario/JSON designed for
performance tests.

test/scenarios/light/scenario_Layer_base_pipeline.json (1)

57-85: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Separate the pipeline wiring check from the ArtNet benchmark.

This step mixes deterministic construction coverage with network-backed throughput assertions, so failures will reflect lab conditions as much as code changes. Keep the module wiring assertions in this scenario and move the ArtNet FPS contract into a dedicated benchmark/perf scenario that does not gate normal test runs.

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

🤖 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/light/scenario_Layer_base_pipeline.json` around lines 57 - 85,
The scenario mixes deterministic wiring checks with network/timing benchmarks:
remove the performance assertions from this "add-artnet" add_module step (fields
measure, bounds.fps, contract and observed entries specific to pc-macos and any
tick_us/at values) so the scenario only verifies module wiring (keep
"name":"add-artnet", "op":"add_module", "id":"ArtNet",
"type":"ArtNetSendDriver", "parent_id":"Drivers" and any non-timing metadata);
then create a separate benchmark/perf scenario that contains the removed
performance-related fields (measure:true, bounds.fps, contract {pc-macos...},
observed {pc-macos...}) so network-backed FPS assertions run only in the
dedicated perf suite and do not gate normal tests.

test/scenarios/light/scenario_Layer_memory_1to1.json (1)

59-86: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Assert the identity-path memory behavior directly instead of timing it over ArtNet.

The regression this scenario describes is structural, but the implementation checks it through a network driver plus timing contracts, which makes the test nondeterministic. Replace the ArtNet-backed measurement with deterministic assertions about LUT/output-buffer state, and keep performance numbers in a separate benchmark path if you still need them.

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

🤖 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/light/scenario_Layer_memory_1to1.json` around lines 59 - 86,
This scenario uses an ArtNetSendDriver measurement (module "add-artnet", id
"ArtNet", type "ArtNetSendDriver", measure: true with bounds/contract/observed)
to detect a structural regression via timing/network, which is nondeterministic;
change the scenario to remove the network/timing measurement (set measure to
false and remove bounds/contract/observed) and instead add deterministic
assertions that verify identity-path memory behavior directly (e.g., explicit
checks of LUT and output-buffer state/identity such as expected LUT contents or
buffer hashes and presence/absence of copies), and if you want to preserve
performance numbers move the fps bounds/contract/observed into a separate
benchmark scenario. Ensure the new assertion keys replace the timing fields so
the test asserts memory/LUT/output-buffer identity deterministically rather than
relying on ArtNet timing.

test/scenarios/light/scenario_MirrorModifier_memory_lut.json (1)

68-95: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Test the LUT allocation path without a network/timing dependency.

This scenario's value is proving the heavy allocation path exists, but the current check routes that through ArtNet and timing contracts, which makes the test environment-sensitive. Assert the LUT/output-buffer allocation directly here and keep ArtNet performance tracking in a separate benchmark-only flow.

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

🤖 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/light/scenario_MirrorModifier_memory_lut.json` around lines 68
- 95, This scenario currently routes the heavy LUT/output-buffer allocation test
through the ArtNet network/timing measurement (module "add-artnet", id "ArtNet",
type "ArtNetSendDriver") which makes it environment-sensitive; remove the
timing/network-dependent keys ("measure": true, "bounds", "contract", and
"observed") from that module and instead assert the LUT/output-buffer allocation
directly in this scenario (e.g., add a lightweight assertion block like
"assertions": {"lut_allocated": true} or a dedicated module named
"check-lut-allocation") so the test no longer depends on network/timing; keep
any ArtNet performance tracking in a separate benchmark-only scenario.

test/unit/light/unit_Layers_container.cpp (1)

30-76: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Always restore the global test clock, even on REQUIRE failure.

setTestNowMs(1000) mutates process-wide state, but Lines 72-73 can abort the test before Line 76 runs. A failing size assertion would leak the pinned clock into later tests and create hard-to-diagnose follow-on failures. Wrap the reset in a small guard so teardown is unconditional.

Suggested guard

-    mm::platform::setTestNowMs(1000);
+    struct ResetTestClock {
+        ~ResetTestClock() { mm::platform::setTestNowMs(0); }
+    } resetTestClock;
+    mm::platform::setTestNowMs(1000);
@@
-    mm::platform::setTestNowMs(0);

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

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_Layers_container.cpp` around lines 30 - 76, The test
sets global time with mm::platform::setTestNowMs(1000) but only resets it at the
end, which can be skipped on a failing REQUIRE; wrap the reset in a scope-guard
so it always runs: introduce a small RAII guard type (e.g., ClockGuard) whose
constructor saves nothing and whose destructor calls
mm::platform::setTestNowMs(0), instantiate it immediately after calling
setTestNowMs(1000), and remove the standalone setTestNowMs(0) at the end; this
ensures mm::platform::setTestNowMs is always restored even if
bareLayer/childLayer assertions fail.

🤖 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/tests/scenario-tests.md`:
- Around line 415-424: The `mdns-off` scenario is marked as passing but the
latest observation for `esp32-eth-wifi` shows tick 106.9ms which exceeds the
contract ceiling of 100.0ms; update the docs to reflect the regression by either
(A) adjusting the Contract/prose for `mdns-off` (e.g., note current regression
or change ceiling values for `esp32-eth-wifi`) or (B) re-run and refresh the
Observed values when the scenario actually meets the existing contract;
reference the `mdns-off` section and the `esp32-eth-wifi` observed tick/heap
entries when making the change.

In `@docs/tests/unit-tests.md`:
- Line 41: The generated docs have truncated bullets in the "Buffer" and
"platform" sections (and similarly at the area corresponding to lines 367-371)
because the doc generator or the source comments are dropping the leading
clause; locate the upstream comment or doc-gen template that produces those
lists (search for the "Buffer" and "platform" doc sources or the generator that
processes unit-test doc comments), fix the generator logic or source text so it
preserves the full bullet text (ensure leading clauses aren't stripped when
splitting/templating), then regenerate docs/tests/unit-tests.md so the bullets
are complete and consistent.

In `@scripts/docs/generate_test_docs.py`:
- Around line 108-134: The helpers _format_contract_line and
_format_observed_line drop free_heap when it is 0 because they check the field
by truthiness; change those checks to test for presence (e.g., "if 'free_heap'
in c" / "if 'free_heap' in o" or "if o.get('free_heap') is not None") so a value
of 0 is rendered, then continue to format using int(c['free_heap']) /
int(o['free_heap']) as before; update both functions (_format_contract_line,
_format_observed_line) to use the explicit presence check and keep the existing
_fmt_heap(...) call to produce the same output for zero.

In `@scripts/scenario/run_live_scenario.py`:
- Around line 201-244: The pre-flight currently only validates ids in
scenario.get("steps") but ignores scenario.get("reset"); update the pre-flight
logic (after computing live_names via _collect_module_names(live_state)) to also
gather ids from reset_steps = scenario.get("reset", []) (and/or inspect
r_step["id"] inside the reset loop) and treat any reset ids missing from
live_names as a hard fail: print the same failure message, set results["passed"]
= False and return results. Ensure you reference the existing symbols target,
live_state, _detect_target, _collect_module_names, reset_steps, and r_step when
adding the check and keep the current exception handling paths intact.
- Around line 318-407: The measurement/contract block must be skipped when the
step actually failed; update the code right before the "if step.get('measure')
or op == 'measure':" check to short-circuit if the step_result indicates failure
(e.g., if step_result.get("ok") is False or step_result.get("status") in
("failed","rejected")), so nothing about observations, contracts, or
--update-contract runs for a step that didn't complete; use the existing
step_result, step, update_contract and wrote_observations symbols to gate the
whole measurement block and return/continue early when the step failed.

In `@scripts/scenario/run_scenario.py`:
- Around line 86-120: The file is opened for read/write without an explicit
encoding which can cause mojibake on non-UTF-8 locales; update the read and
write calls that use the path variable (where json.load is called and later
json.dump is used) to open files explicitly with encoding="utf-8" (e.g., use the
same read/open and write/open sites around json.load(scenario) and
json.dump(scenario, ...) so both the initial open(path) and the later open(path,
"w") specify UTF-8) to ensure fixtures with non-ASCII characters are preserved.

In `@test/scenario_runner.cpp`:
- Around line 514-520: The in-process runner incorrectly treats min_pct as a
no-op (setting fpsBound = 1); update the branch in scenario_runner.cpp that
checks step["bounds"]["fps"].has("min_pct") so it computes an absolute fpsBound
from the percentage instead of using 1 — e.g., multiply min_pct.num by the
appropriate reference FPS for that scenario (use the scenario's reference/target
FPS variable or the baseline/previous-measured FPS available in the runner) and
set fpsBound = ceil(result); modify the code around fpsBound and fpsLedProduct
assignments so min, min_pct and min_fps_led_product are all handled correctly
and consistently.
- Around line 544-553: The MEASURE log currently prints a bytes-consumed delta
(heapDelta = heapBefore - heapAfterMeasure) but consumers expect an absolute
free-heap value; update the MEASURE output in scenario_runner.cpp so the printed
"heap=" is the absolute free heap at measurement (heapAfterMeasure) instead of
heapBefore - heapAfterMeasure, keeping the existing stepDelta (heapAfter -
heapAfterMeasure) unchanged; locate the printf call that prints name,
tickTimeUs, fps, lights, heapDelta, stepDelta and replace the heap value with
the absolute heapAfterMeasure (or adjust heapDelta to be casted
heapAfterMeasure) so run_scenario.py gets the correct
observed.<target>.free_heap.

In `@test/scenarios/light/scenario_GridLayout_grid_sizes.json`:
- Around line 65-78: This scenario currently depends on network I/O and strict
timing baselines because it adds modules with id "ArtNet" of type
"ArtNetSendDriver" and "Preview" of type "PreviewDriver" under "Drivers"; remove
or replace the "ArtNet" module and any timing/throughput assertions so tests no
longer require network or absolute FPS/tick/heap numbers, instead swap the
renderer/sink to a local non-network driver (e.g., a null or in-memory sink)
while keeping mutation coverage for the scenario, and move any throughput/timing
baselines into an opt-in benchmark/performance suite so normal tests remain
deterministic and host-independent.

In `@test/scenarios/light/scenario_GridLayout_resize.json`:
- Around line 61-67: The scenario is flaky because it adds a real network driver
("fix-artnet" add_module with id "ArtNet", type "ArtNetSendDriver", parent_id
"Drivers") and relies on timing/heap measurements; remove or replace that real
ArtNet module with a deterministic stub/mock driver (or omit the add_module
block) and delete any tick/heap/timing assertions or benchmark steps in the same
scenario (the region referenced around lines 100-223) so the resize/reallocation
verification only exercises deterministic layout logic; move the
ArtNet/tick/heap benchmarking into a separate non-gating benchmark flow.

In `@test/scenarios/light/scenario_Layer_buildup.json`:
- Around line 54-77: The "measure-minimum" scenario (op: "measure", measure:
true) is a performance benchmark and should be removed from the required
scenario suite: delete or relocate the entire scenario object (including
"contract" and "observed" blocks with tick_us/free_heap and any ArtNet sink
references) into a dedicated benchmark/performance workflow, leaving only the
functional step-by-step construction assertions in this file; update or remove
the fields "measure", "contract", and "observed" for the entries at the shown
ranges (including the other affected blocks around lines 89-95 and 98-203) so
the remaining tests do not depend on timing or network and keep the profiling
checks in an opt-in benchmark track.

In `@test/unit/core/unit_platform_clock.cpp`:
- Around line 19-27: The test asserts a > 0 which assumes real wall-clock
progress; instead remove that timing-dependent check and only verify that
mm::platform::millis() no longer returns the pinned override values after
calling mm::platform::setTestNowMs(0). Update the test around variables a and b
(from the mm::platform::millis() calls) to drop CHECK(a > 0) and keep only the
checks that a != 12345 and b != 67890 so the test no longer depends on startup
timing.

In `@test/unit/light/unit_ArtNetSendDriver_no_alloc_in_loop.cpp`:
- Around line 21-88: The tests validate resizing in
onBuildState()/onCorrectionChanged() but never exercise loop(), so add at least
one assertion after calling driver.loop() to ensure no heap allocation happens
on the hot path: call driver.loop() (or the appropriate transmit/send entry used
by ArtNetSendDriver) in one of the TEST_CASEs (e.g., "ArtNetSendDriver sizes
corrected_ in onBuildState, not in loop" or the no-op case), then assert that
driver.correctedBuffer().data() is unchanged and the buffer shape (count and
channelsPerLight) remains the same as before the loop call to pin the contract
that loop() does not reallocate corrected_. Ensure you reference driver.loop()
and correctedBuffer() in the change.

---

Outside diff comments:
In `@scripts/docs/_test_metadata.py`:
- Around line 114-123: list_test_modules() only collects primary "module" values
from collect_unit_files() and collect_scenario_files(), so modules referenced
only via the secondary "`@also`"/"also" field are omitted; update
list_test_modules to also extract and add any entries from the secondary modules
field (e.g., f.get("also") / s.get("also") or whatever key the file dict uses
for `@also`) into the modules set when iterating over collect_unit_files() and
collect_scenario_files(), normalizing them to strings before adding and then
return sorted(modules).

In `@scripts/moondeck.py`:
- Around line 657-682: The _serve_doc_asset handler currently serves any file
under ROOT; restrict it to only renderer-needed doc asset directories and safe
file types: add a whitelist of allowed subdirectories (e.g., "docs", "site",
"assets" or whatever your markdown renderer uses) and ensure
asset_path.relative_to(ROOT / allowed_dir) succeeds for one of those allowed
dirs, and also enforce a whitelist of allowed extensions/mime types (images,
css, js) before reading/writing; additionally update main() so the server binds
to localhost (127.0.0.1) or make the bind host configurable instead of listening
on all interfaces to prevent LAN access. Ensure you change checks in
_serve_doc_asset, reference ROOT and asset_path, and update main()'s server bind
call accordingly.

In `@src/light/drivers/Drivers.h`:
- Around line 102-103: Add a one-line exception-reason comment immediately above
the MoonModule::onBuildState() call to document that this override intentionally
invokes the parent first (after passBufferToDrivers()) so child drivers run
their onBuildState() with setSourceBuffer() and setCorrection() already wired;
mention explicitly that this reverses the new default ordering on purpose to
prevent future maintainers from “fixing” the order back.

In `@src/light/layers/Layer.h`:
- Around line 80-87: The empty-layer branch that runs when physicalCount == 0
clears widths, frees lut_ and buffer_, and clears status but does not reset
lutSkipped_, so a previously degraded state can still report lutSkipped()==true;
update the branch (near physicalCount == 0 handling where
physicalWidth_/physicalHeight_/physicalDepth_, width_/height_/depth_,
lut_.free(), buffer_.free(), setDynamicBytes(0), clearStatus(),
MoonModule::onBuildState() are called) to explicitly set lutSkipped_ = false so
rebuildLUT() being skipped won't leave stale lutSkipped_ state.

In `@test/scenario_runner.cpp`:
- Around line 281-303: The PreviewDriver handling is incorrectly nested inside
the if (step.has("props")) block so setPreviewFrame() is never called for
fixtures that lack props; move or duplicate the PreviewDriver branch so that
when std::strcmp(type, "PreviewDriver") == 0 you always call
static_cast<mm::PreviewDriver*>(mod)->setPreviewFrame(&scenarioPreviewFrame())
regardless of step.has("props"), e.g., extract the PreviewDriver branch out of
the props conditional (or add a separate check after it) so setPreviewFrame and
scenarioPreviewFrame are always invoked for PreviewDriver modules.

In `@test/scenarios/light/scenario_Layer_base_pipeline.json`:
- Around line 57-85: The scenario mixes deterministic wiring checks with
network/timing benchmarks: remove the performance assertions from this
"add-artnet" add_module step (fields measure, bounds.fps, contract and observed
entries specific to pc-macos and any tick_us/at values) so the scenario only
verifies module wiring (keep "name":"add-artnet", "op":"add_module",
"id":"ArtNet", "type":"ArtNetSendDriver", "parent_id":"Drivers" and any
non-timing metadata); then create a separate benchmark/perf scenario that
contains the removed performance-related fields (measure:true, bounds.fps,
contract {pc-macos...}, observed {pc-macos...}) so network-backed FPS assertions
run only in the dedicated perf suite and do not gate normal tests.

In `@test/scenarios/light/scenario_Layer_memory_1to1.json`:
- Around line 59-86: This scenario uses an ArtNetSendDriver measurement (module
"add-artnet", id "ArtNet", type "ArtNetSendDriver", measure: true with
bounds/contract/observed) to detect a structural regression via timing/network,
which is nondeterministic; change the scenario to remove the network/timing
measurement (set measure to false and remove bounds/contract/observed) and
instead add deterministic assertions that verify identity-path memory behavior
directly (e.g., explicit checks of LUT and output-buffer state/identity such as
expected LUT contents or buffer hashes and presence/absence of copies), and if
you want to preserve performance numbers move the fps bounds/contract/observed
into a separate benchmark scenario. Ensure the new assertion keys replace the
timing fields so the test asserts memory/LUT/output-buffer identity
deterministically rather than relying on ArtNet timing.

In `@test/scenarios/light/scenario_MirrorModifier_memory_lut.json`:
- Around line 68-95: This scenario currently routes the heavy LUT/output-buffer
allocation test through the ArtNet network/timing measurement (module
"add-artnet", id "ArtNet", type "ArtNetSendDriver") which makes it
environment-sensitive; remove the timing/network-dependent keys ("measure":
true, "bounds", "contract", and "observed") from that module and instead assert
the LUT/output-buffer allocation directly in this scenario (e.g., add a
lightweight assertion block like "assertions": {"lut_allocated": true} or a
dedicated module named "check-lut-allocation") so the test no longer depends on
network/timing; keep any ArtNet performance tracking in a separate
benchmark-only scenario.

In `@test/scenarios/light/scenario_MirrorModifier_pipeline.json`:
- Around line 64-91: The scenario "add-artnet" (id "ArtNet", type
"ArtNetSendDriver") currently enables measurement and a bounded FPS contract
(fields measure, bounds, contract, observed) which makes the test depend on
network/timing; remove or disable those measurement/contract fields from this
scenario so it only validates mirror pipeline wiring (keep the module addition
and end-to-end construction/assertion flow intact), and move the throughput
contract and observed metrics into a separate benchmark scenario/JSON designed
for performance tests.

In `@test/unit/light/unit_Layers_container.cpp`:
- Around line 30-76: The test sets global time with
mm::platform::setTestNowMs(1000) but only resets it at the end, which can be
skipped on a failing REQUIRE; wrap the reset in a scope-guard so it always runs:
introduce a small RAII guard type (e.g., ClockGuard) whose constructor saves
nothing and whose destructor calls mm::platform::setTestNowMs(0), instantiate it
immediately after calling setTestNowMs(1000), and remove the standalone
setTestNowMs(0) at the end; this ensures mm::platform::setTestNowMs is always
restored even if bareLayer/childLayer assertions fail.

🪄 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: d6ac05b2-48c5-4c4b-98b6-081a287f3d06

📥 Commits

Reviewing files that changed from the base of the PR and between 25d5db3 and c881578.

📒 Files selected for processing (44)

• CLAUDE.md
• README.md
• docs/coding-standards.md
• docs/moonmodules/light/EffectBase.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/effects/GlowParticlesEffect.md
• docs/moonmodules/light/effects/LavaLampEffect.md
• docs/moonmodules/light/effects/RipplesEffect.md
• docs/performance.md
• docs/plan.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• scripts/docs/_test_metadata.py
• scripts/docs/generate_test_docs.py
• scripts/moondeck.py
• scripts/scenario/run_live_scenario.py
• scripts/scenario/run_scenario.py
• src/light/drivers/ArtNetSendDriver.h
• src/light/drivers/Drivers.h
• src/light/layers/Layer.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/platform.h
• 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_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_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/scenarios/light/scenario_PreviewDriver_detail.json
• test/unit/core/unit_Buffer.cpp
• test/unit/core/unit_MappingLUT.cpp
• test/unit/core/unit_platform_clock.cpp
• test/unit/light/unit_ArtNetSendDriver_no_alloc_in_loop.cpp
• test/unit/light/unit_CheckerboardEffect.cpp
• test/unit/light/unit_Layer_phase_animation.cpp
• test/unit/light/unit_Layers_container.cpp
• test/unit/light/unit_Layouts_toggle_cycle.cpp

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

test/scenarios/light/scenario_PreviewDriver_detail.json (1)

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

Rewrite these step descriptions in present tense.

generate_test_docs.py copies these strings verbatim into docs/tests/scenario-tests.md, so phrases like “must still hit” and “must not affect” leak forward-looking language into docs/. Please phrase them as current-state statements instead.

As per coding guidelines, “Forward-looking language (roadmap, will be, planned, in the future, currently lacks, TODO, FIXME) must not appear in docs/ (except docs/plan.md and docs/history/) or src/ code.”

Also applies to: 295-295, 397-397, 499-499

🤖 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/light/scenario_PreviewDriver_detail.json` at line 193, Update
the "description" strings in
test/scenarios/light/scenario_PreviewDriver_detail.json to use present-tense
phrasing instead of forward-looking language: replace phrases like "must still
hit" and "must not affect" with present-state statements (e.g., "hits 80% of
baseline" or "does not affect X") for the entries currently at the described
positions (the JSON objects containing the "description" keys around the lines
noted — including the entries referenced at the other occurrences). Ensure all
four affected "description" values (the one shown and the ones at the other
noted locations) are rewritten to present tense so generate_test_docs.py emits
current-state language into docs.

🤖 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/tests/scenario-tests.md`:
- Around line 513-523: The generated PreviewDriver tables in scenario-tests.md
are out of sync with the source scenario_PreviewDriver_detail.json: regenerate
the tables from that JSON (re-run the doc/table generator used for
PreviewDriver) so the observed["esp32"] entries produce an "esp32" row in each
measured-step table; ensure the generator logic that emits rows for targets
includes the "esp32" key (and re-run for the other PreviewDriver sections
referenced) so all telemetry rows are present and the docs match the JSON
source.

In `@docs/tests/unit-tests.md`:
- Around line 12-14: Update the three generated bullets to use present-tense
wording: change “would need”→“needs” (in the onBuildState/loop() sentence),
“grow/grows/grow happens”→use present tense like “grows” or “is grown”
consistently for the preset toggle sentence describing onCorrectionChanged and
outChannels, and change “would have caught” (if present in the other section
lines 94–100) to present-tense like “catches”; keep references to onBuildState,
loop(), onCorrectionChanged, and outChannels intact and ensure the text states
the current contract (no forward-looking language) and regenerate the docs page
accordingly.

In `@scripts/check/collect_kpi.py`:
- Around line 42-48: The failure banner still mentions "18 FPS at the 128×128 /
16384-light reference" even though MIN_ESP32_FPS_LED_PRODUCT was lowered to 10
FPS; update the banner text to reflect the new 10 FPS floor. Locate the failure
message print/format that references the 18 FPS baseline (in
scripts/check/collect_kpi.py) and change it to mention 10 FPS (or derive the
value from MIN_ESP32_FPS_LED_PRODUCT divided by 16384) and keep the rest of the
context (128×128 / 16384-light reference) intact so failing runs report the
correct baseline.

In `@scripts/docs/generate_test_docs.py`:
- Around line 154-176: _format_perf_table currently displays raw nominal
contract values (tick_us, free_heap, max_alloc_block) via
_fps_floor_from_contract and _heap_contract_cell which confuses readers because
runtime uses per-target tolerances; update _format_perf_table to show the
tolerated contract bound (or show "nominal / tolerated" together) instead of the
raw nominal: for FPS use a tolerance-aware helper (e.g.
_fps_tolerated_from_contract or compute tolerated tick_us from
contract.get('tick_us') plus its tolerance) in place of
_fps_floor_from_contract, and for heap/block use tolerance-aware cells (e.g.
_heap_tolerated_cell or compute tolerated free_heap/max_alloc_block from the
contract tolerances) in place of _heap_contract_cell; ensure the FPS/heap/block
cell strings include the tolerated bound (or explicitly label the nominal vs
tolerated) and update the header line text if needed to indicate tolerated
values are shown.

In `@scripts/moondeck.py`:
- Around line 183-199: Replace the inline try/except blocks that only call
_LAST_FLASH_FILE.unlink() and swallow OSError with contextlib.suppress to
satisfy Ruff E701/SIM105: in _consume_last_flash() (the unlink call after TTL
check) change the try/except around _LAST_FLASH_FILE.unlink() to use with
suppress(OSError): _LAST_FLASH_FILE.unlink(), and make the analogous change for
the other unlink site around lines ~246–247; similarly, for the handle() method
that currently catches ConnectionResetError/BrokenPipeError, wrap the
super().handle() call in with suppress(ConnectionResetError, BrokenPipeError):
super().handle() instead of an inline try/except.

In `@scripts/scenario/run_live_scenario.py`:
- Around line 400-409: The check currently skips enforcement when max_block ==
0; change the logic in the max_alloc_block handling (variables exp_block,
max_block, heap_tol_pct, and results["passed"]) to treat a zero max_block as a
failing condition when an expected exp_block (>0) exists: remove the max_block >
0 short-circuit and explicitly fail (set results["passed"]=False and print a
clear FAIL message) when max_block <= 0, otherwise compute drop_pct as before
and compare to heap_tol_pct to decide pass/fail; keep the existing PASS/FAIL
message formats and context.

---

Outside diff comments:
In `@test/scenarios/light/scenario_PreviewDriver_detail.json`:
- Line 193: Update the "description" strings in
test/scenarios/light/scenario_PreviewDriver_detail.json to use present-tense
phrasing instead of forward-looking language: replace phrases like "must still
hit" and "must not affect" with present-state statements (e.g., "hits 80% of
baseline" or "does not affect X") for the entries currently at the described
positions (the JSON objects containing the "description" keys around the lines
noted — including the entries referenced at the other occurrences). Ensure all
four affected "description" values (the one shown and the ones at the other
noted locations) are rewritten to present tense so generate_test_docs.py emits
current-state language into docs.

🪄 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: c45e4e93-be33-49c9-a52d-249854609a00

📥 Commits

Reviewing files that changed from the base of the PR and between c881578 and a1db506.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (29)

• .gitignore
• docs/plan.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• scripts/check/collect_kpi.py
• scripts/docs/_test_metadata.py
• scripts/docs/generate_test_docs.py
• scripts/moondeck.py
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/style.css
• scripts/scenario/_observed.py
• scripts/scenario/run_live_scenario.py
• scripts/scenario/run_scenario.py
• src/core/NetworkModule.h
• src/light/layers/Layer.h
• test/scenario_runner.cpp
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/core/scenario_NetworkModule_mdns_toggle.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_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/scenarios/light/scenario_PreviewDriver_detail.json
• test/unit/light/unit_ArtNetSendDriver_no_alloc_in_loop.cpp
• test/unit/light/unit_Layers_container.cpp

[coderabbitai]

Actionable comments posted: 4

Caution

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

⚠️ Outside diff range comments (2)

scripts/scenario/run_scenario.py (1)

125-137: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Renegotiation should refresh opted-in max_alloc_block contracts too.

When a scenario already asserts max_alloc_block, --update-contract leaves that floor unchanged while updating the rest of the contract. That produces a partially-renegotiated block where tick_us/free_heap/set_by describe the new run but the memory floor still describes the old one. As per coding guidelines, contract[<target>] is an absolute cross-run promise.

Suggested fix

-            for k in ("tick_tolerance_pct", "heap_tolerance_pct", "tolerance_us",
-                      "max_alloc_block"):
+            for k in ("tick_tolerance_pct", "heap_tolerance_pct", "tolerance_us"):
                 if k in existing:
                     new_block[k] = existing[k]
+            if "max_alloc_block" in existing:
+                new_block["max_alloc_block"] = observations[name]["max_alloc_block"]

🤖 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_scenario.py` around lines 125 - 137, The update path
currently preserves an old max_alloc_block value by copying it from existing
into new_block; instead, when --update-contract is used and the contract
previously opted into max_alloc_block, refresh that field from the current run.
Change the contract update code (the block that builds new_block in the
update_contract branch) so the tuple for copying existing keys excludes
"max_alloc_block", and add a separate conditional: if "max_alloc_block" is
present in existing then set new_block["max_alloc_block"] =
observations[name]["max_alloc_block"]; keep the existing behavior for
tick_tolerance_pct, heap_tolerance_pct, and tolerance_us unchanged and still
copy them from existing.

scripts/scenario/run_live_scenario.py (1)

453-467: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Only commit renegotiated contracts after a successful run, and update the whole asserted block together.

--update-contract rewrites tick_us/free_heap immediately, preserves any existing max_alloc_block, and the final or update_contract writeback still dumps the JSON even when the scenario later fails or no measured step touched the contract. That can stamp a fresh set_by/reason onto stale or failing data. As per coding guidelines, contract[<target>] is an absolute cross-run promise.

Suggested fix

-    wrote_observations = [False]  # sentinel; flipped by each measure step that runs
+    wrote_observations = [False]  # sentinel; flipped by each measure step that runs
+    wrote_contracts = [False]
@@
             if update_contract:
                 # Preserve any per-step tolerance overrides already in place.
                 existing = step.get("contract", {}).get(target, {})
                 new_block = {
                     "tick_us": int(tick_us),
                     "free_heap": int(heap),
                     "set_by": _today_iso(),
                     "reason": update_reason or existing.get("reason", "updated"),
                 }
-                for k in ("tick_tolerance_pct", "heap_tolerance_pct", "tolerance_us",
-                          "max_alloc_block"):
+                for k in ("tick_tolerance_pct", "heap_tolerance_pct", "tolerance_us"):
                     if k in existing:
                         new_block[k] = existing[k]
+                if "max_alloc_block" in existing:
+                    new_block["max_alloc_block"] = int(max_block)
                 step.setdefault("contract", {})[target] = new_block
+                wrote_contracts[0] = True
@@
-    if wrote_observations[0] or update_contract:
+    if results["passed"] and (wrote_observations[0] or wrote_contracts[0]):
         with open(scenario_path, "w") as f:
             json.dump(scenario, f, indent=2, ensure_ascii=False)
             f.write("\n")
         what = []
         if wrote_observations[0]:
             what.append(f"observed[{target}]")
-        if update_contract:
+        if wrote_contracts[0]:
             what.append(f"contract[{target}]")

Also applies to: 523-532

🤖 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 453 - 467, The current
code writes contract[<target>] immediately when update_contract is true, which
can stamp set_by/reason onto stale or failed runs; instead, collect pending
contract updates (build new_block from existing and copy per-step keys like
tick_tolerance_pct, heap_tolerance_pct, tolerance_us, max_alloc_block) into an
in-memory map keyed by (step, target) during the run and do not call
step.setdefault("contract", {})[target] yet; after the scenario completes
successfully and only for steps that were actually measured/touched, iterate the
pending updates and write the entire asserted block to
step.setdefault("contract", {})[target] (setting tick_us, free_heap, set_by via
_today_iso(), and reason) so the contract is only committed on success; apply
the same buffering/commit change to the other mirrored block that currently
appears later (the similar code at lines ~523-532).

♻️ Duplicate comments (5)

scripts/docs/generate_test_docs.py (2)

137-141: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Render free_heap: 0 explicitly as "unlimited" instead of "—".

Desktop contracts use free_heap: 0 as a real sentinel meaning unlimited heap. The current check if v in (None, 0): return "—" treats zero the same as missing, causing desktop contract rows to omit the heap clause entirely in generated docs. This creates a mismatch between the scenario JSON, the runtime contract semantics documented in docs/testing.md, and the rendered inventory.

🔧 Proposed fix

 def _heap_contract_cell(v) -> str:
     """Contract heap/block floor → '≥ N KB'. Missing or 0 → '—'."""
-    if v in (None, 0):
+    if v is None:
         return "—"
+    if v == 0:
+        return "unlimited"
     return f"≥ {_fmt_heap(int(v))}"

Apply the same fix to _heap_observed_cell around lines 144-154 if it treats zero as missing.

🤖 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/docs/generate_test_docs.py` around lines 137 - 141, The helper
_heap_contract_cell currently treats v==0 like missing; change its logic so only
None is rendered as "—" and v==0 is rendered explicitly as "unlimited" (e.g.,
return "unlimited" when v == 0), while other values continue to use f"≥
{_fmt_heap(int(v))}"; apply the same adjustment to _heap_observed_cell so
observed free_heap==0 is also rendered as "unlimited" and not as missing.

---

157-200: ⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

Show tolerated contract bounds instead of raw nominal values in the performance table.

The _format_perf_table function renders tick_us / free_heap / max_alloc_block as hard thresholds via _fps_floor_from_contract and _heap_contract_cell, but the live runner enforces those fields with per-target tolerances. The generated docs therefore show apparent contract breaches for runs that still pass at runtime—for example, some target rows may render ≥ 3,333 FPS while the observed range tops out below that threshold but within tolerance.

Please update the table to show the tolerated bound (or display both nominal and tolerated values together) so readers understand what the runtime actually enforces.

📋 Suggested approach

For FPS: compute the tolerated tick ceiling from contract.get('tick_us') plus its tolerance, then pass that to a tolerance-aware helper (or adjust _fps_floor_from_contract to accept a tolerance parameter).

For heap/block: compute the tolerated floor from contract.get('free_heap') / contract.get('max_alloc_block') minus their tolerances, then render those values instead of the raw nominal.

Alternatively, show both values in each cell: "≥ 3,000 FPS (nominal) / ≥ 2,850 FPS (tolerated)" to make the distinction explicit.

🤖 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/docs/generate_test_docs.py` around lines 157 - 200, The perf table
shows raw nominal contract values causing apparent breaches; update
_format_perf_table to compute and display tolerance-adjusted bounds when
rendering tick_us, free_heap, and max_alloc_block: for FPS compute tolerated
tick_us = contract.get("tick_us") + tolerance and pass that to
_fps_floor_from_contract (or extend that helper to accept a tolerance), and for
heap/block compute tolerated floors = nominal - tolerance and pass those to
_heap_contract_cell (or a new param) or render both nominal and tolerated values
in the cell (e.g. "nominal / tolerated") so that _fps_range_from_observed_range
and _heap_observed_cell remain unchanged but the contract side reflects runtime
tolerances; update callers in _format_perf_table to use these tolerated values
for fps, heap, and block cells.

test/scenario_runner.cpp (2)

553-563: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

MEASURE output still prints heap delta instead of absolute free heap.

Lines 553-559 compute heapDelta = heapBefore - heapAfterMeasure and print heap=%+ld. If run_scenario.py parses this field as the absolute free heap for observed.<target>.free_heap storage (as documented in docs/testing.md lines 167-191), this will write incorrect values into scenario JSON files.

The past review comment flagged the same issue. Either:

1. Change line 559 to print heapAfterMeasure as heap= (absolute value), or
2. Confirm that run_scenario.py correctly interprets the signed delta format.

🔧 Suggested fix if absolute value is intended

-            long heapDelta = heapBefore > 0
-                ? static_cast<long>(heapBefore) - static_cast<long>(heapAfterMeasure)
-                : 0;
+            long heapValue = heapAfterMeasure > 0
+                ? static_cast<long>(heapAfterMeasure)
+                : 0;
             long stepDelta = heapBefore > 0
                 ? static_cast<long>(heapAfter) - static_cast<long>(heapAfterMeasure)
                 : 0;
-            std::printf("  MEASURE %s: tick=%uus FPS=%u lights=%u heap=%+ld (step: %+ld) block=%u\n",
+            std::printf("  MEASURE %s: tick=%uus FPS=%u lights=%u heap=%ld (step: %+ld) block=%u\n",
                         name,
                         static_cast<unsigned>(tickTimeUs), static_cast<unsigned>(fps),
-                        lights, heapDelta, stepDelta,
+                        lights, heapValue, stepDelta,
                         static_cast<unsigned>(maxBlock));

🤖 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 553 - 563, The MEASURE log currently
prints heapDelta (computed as heapDelta = heapBefore - heapAfterMeasure) but
run_scenario.py expects an absolute free heap value; update the printf in
scenario_runner.cpp (the printf that prints name, tickTimeUs, fps, lights,
heap=..., step=...) to output heapAfterMeasure (the absolute free heap) instead
of heapDelta, and keep stepDelta (computed from heapAfter) as the signed step
value; ensure you reference the existing variables heapAfterMeasure, heapDelta,
stepDelta and the printf call when making the change.

---

518-524: ⚠️ Potential issue | 🟡 Minor | 💤 Low value

In-process runner intentionally skips min_pct bounds.

The code sets fpsBound = 1 for min_pct, effectively only checking FPS > 0. The comment indicates this is intentional ("min_pct is for live runner"), but scenarios like scenario_MoonModule_control_change.json declare min_pct: 80 expecting regression detection.

This is a known limitation (already flagged in past review) — in-process scenarios can't enforce relative FPS bounds without a baseline. Consider documenting this limitation in docs/testing.md § bounds vs contract, or adding a skip/warning message when min_pct appears in construct-mode scenarios.

🤖 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 518 - 524, The in-process runner
currently treats bounds.fps.min_pct by setting fpsBound = 1 (only checking FPS >
0), which prevents scenarios like scenario_MoonModule_control_change.json from
enforcing relative FPS regression; modify the handling in scenario_runner.cpp
where step, fpsBound and fpsLedProduct are processed so that when
step["bounds"]["fps"].has("min_pct") in construct/in-process mode you either (a)
emit a clear warning/log (via the existing logger) that min_pct cannot be
enforced in-process and the check is skipped, or (b) mark the scenario/step as
skipped for relative-FPS checks and record that in test output; ensure the
message references the scenario/step id and the provided min_pct value so users
can see why the bound was not applied.

scripts/scenario/run_live_scenario.py (1)

334-337: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle measurement failures inside the step loop.

collect_metrics() sits outside the surrounding try, so a dropped /api/system or /api/state request aborts the whole scenario run and skips the normal cleanup path for created modules.

Suggested fix

         if step.get("measure") or op == "measure":
-            metrics = collect_metrics(client, settle_s)
+            try:
+                metrics = collect_metrics(client, settle_s)
+            except Exception as e:
+                step_result["status"] = "error"
+                step_result["error"] = str(e)
+                print(f"  FAIL  {step_name}: {e}")
+                results["passed"] = False
+                results["steps"].append(step_result)
+                continue
             step_result["metrics"] = metrics

🤖 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 334 - 337, The call to
collect_metrics(client, settle_s) must be protected so metric fetch failures
don’t abort the whole run; inside the step loop (where you check if
step.get("measure") or op == "measure") wrap the collect_metrics(...) call in a
try/except that catches network/HTTP errors, logs a warning, and assigns
step_result["metrics"] = {} (or a safe default) and tick_us = 0 on failure so
the scenario continues and normal cleanup still runs; keep references to
collect_metrics, client, settle_s, step_result, and tick_us when implementing
this change.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/building.md`:
- Line 110: The doc currently mentions a future roadmap; remove the roadmap
sentence and reword the paragraph to describe the present state: state that the
Eth pin map is baked into the build, confirm the Olimex ESP32-Gateway
verification, and note that boards with the same LAN8720 PHY but different
pinouts (e.g., WT32-ETH01 with reset on GPIO16) require a local rebuild today;
eliminate any mention of "2.0 roadmap" or future/planned runtime PHY/pin
selection.

In `@scripts/docs/screenshot_modules.py`:
- Around line 522-534: The orphan-sweep currently swallows all exceptions
(around _get(...) and the recursive _sweep_orphans) which hides failures; change
the try/except to catch exceptions as e and log or print the error (including
context like args.host and module name) instead of a bare pass, and also wrap
individual delete_module(args.host, n) calls in their own try/except to log
failures and continue recursion so one failed delete doesn't abort the whole
_sweep_orphans; apply the same change to the other sweep block that uses
_sweep_orphans/delete_module so all cleanup errors are surfaced.
- Around line 564-579: The branch that prints "skip ui_overview (already
captured)" and appends to skipped runs even when the user filter excludes
"ui_overview"; update the conditional logic around overview_path/args.force/filt
so you only report and append skipped for the "already exists" case when the
filter allows this item (i.e., preserve the existing check "(not filt or filt in
'ui_overview')" before marking skipped). Concretely, in the block guarded by
args.extras_only false, relocate or extend the existing filter test so the else
path that prints and appends to skipped only runs when filt is empty or includes
"ui_overview"; if the filter excludes it, print nothing (or a filtered message)
and do not append to skipped/captured.

In `@scripts/moondeck_ui/style.css`:
- Around line 287-291: The .device-board badge sets max-width: 16ch but doesn't
clamp overflowing text; update the .device-board rule (the .device-board
selector) to ensure long firmware names are truncated by making it a
block/inline-block element and adding overflow: hidden, text-overflow: ellipsis,
and white-space: nowrap so labels remain badge-sized and show an ellipsis when
they exceed max-width.

---

Outside diff comments:
In `@scripts/scenario/run_live_scenario.py`:
- Around line 453-467: The current code writes contract[<target>] immediately
when update_contract is true, which can stamp set_by/reason onto stale or failed
runs; instead, collect pending contract updates (build new_block from existing
and copy per-step keys like tick_tolerance_pct, heap_tolerance_pct,
tolerance_us, max_alloc_block) into an in-memory map keyed by (step, target)
during the run and do not call step.setdefault("contract", {})[target] yet;
after the scenario completes successfully and only for steps that were actually
measured/touched, iterate the pending updates and write the entire asserted
block to step.setdefault("contract", {})[target] (setting tick_us, free_heap,
set_by via _today_iso(), and reason) so the contract is only committed on
success; apply the same buffering/commit change to the other mirrored block that
currently appears later (the similar code at lines ~523-532).

In `@scripts/scenario/run_scenario.py`:
- Around line 125-137: The update path currently preserves an old
max_alloc_block value by copying it from existing into new_block; instead, when
--update-contract is used and the contract previously opted into
max_alloc_block, refresh that field from the current run. Change the contract
update code (the block that builds new_block in the update_contract branch) so
the tuple for copying existing keys excludes "max_alloc_block", and add a
separate conditional: if "max_alloc_block" is present in existing then set
new_block["max_alloc_block"] = observations[name]["max_alloc_block"]; keep the
existing behavior for tick_tolerance_pct, heap_tolerance_pct, and tolerance_us
unchanged and still copy them from existing.

---

Duplicate comments:
In `@scripts/docs/generate_test_docs.py`:
- Around line 137-141: The helper _heap_contract_cell currently treats v==0 like
missing; change its logic so only None is rendered as "—" and v==0 is rendered
explicitly as "unlimited" (e.g., return "unlimited" when v == 0), while other
values continue to use f"≥ {_fmt_heap(int(v))}"; apply the same adjustment to
_heap_observed_cell so observed free_heap==0 is also rendered as "unlimited" and
not as missing.
- Around line 157-200: The perf table shows raw nominal contract values causing
apparent breaches; update _format_perf_table to compute and display
tolerance-adjusted bounds when rendering tick_us, free_heap, and
max_alloc_block: for FPS compute tolerated tick_us = contract.get("tick_us") +
tolerance and pass that to _fps_floor_from_contract (or extend that helper to
accept a tolerance), and for heap/block compute tolerated floors = nominal -
tolerance and pass those to _heap_contract_cell (or a new param) or render both
nominal and tolerated values in the cell (e.g. "nominal / tolerated") so that
_fps_range_from_observed_range and _heap_observed_cell remain unchanged but the
contract side reflects runtime tolerances; update callers in _format_perf_table
to use these tolerated values for fps, heap, and block cells.

In `@scripts/scenario/run_live_scenario.py`:
- Around line 334-337: The call to collect_metrics(client, settle_s) must be
protected so metric fetch failures don’t abort the whole run; inside the step
loop (where you check if step.get("measure") or op == "measure") wrap the
collect_metrics(...) call in a try/except that catches network/HTTP errors, logs
a warning, and assigns step_result["metrics"] = {} (or a safe default) and
tick_us = 0 on failure so the scenario continues and normal cleanup still runs;
keep references to collect_metrics, client, settle_s, step_result, and tick_us
when implementing this change.

In `@test/scenario_runner.cpp`:
- Around line 553-563: The MEASURE log currently prints heapDelta (computed as
heapDelta = heapBefore - heapAfterMeasure) but run_scenario.py expects an
absolute free heap value; update the printf in scenario_runner.cpp (the printf
that prints name, tickTimeUs, fps, lights, heap=..., step=...) to output
heapAfterMeasure (the absolute free heap) instead of heapDelta, and keep
stepDelta (computed from heapAfter) as the signed step value; ensure you
reference the existing variables heapAfterMeasure, heapDelta, stepDelta and the
printf call when making the change.
- Around line 518-524: The in-process runner currently treats bounds.fps.min_pct
by setting fpsBound = 1 (only checking FPS > 0), which prevents scenarios like
scenario_MoonModule_control_change.json from enforcing relative FPS regression;
modify the handling in scenario_runner.cpp where step, fpsBound and
fpsLedProduct are processed so that when step["bounds"]["fps"].has("min_pct") in
construct/in-process mode you either (a) emit a clear warning/log (via the
existing logger) that min_pct cannot be enforced in-process and the check is
skipped, or (b) mark the scenario/step as skipped for relative-FPS checks and
record that in test output; ensure the message references the scenario/step id
and the provided min_pct value so users can see why the bound was not applied.

🪄 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: faa2d421-f3ed-44d7-9f6f-7c7db0d81d11

📥 Commits

Reviewing files that changed from the base of the PR and between c881578 and 302c5e6.

⛔ Files ignored due to path filters (10)

• docs/assets/screenshots/moondeck_esp32.png is excluded by !**/*.png
• docs/assets/screenshots/moondeck_live.png is excluded by !**/*.png
• docs/assets/screenshots/moondeck_pc.png is excluded by !**/*.png
• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/clean_esp32.py is excluded by !**/build/**
• scripts/build/flash_esp32.py is excluded by !**/build/**
• scripts/build/generate_build_info.py is excluded by !**/build/**
• scripts/build/generate_manifest.py is excluded by !**/build/**
• scripts/build/host_wifi.py is excluded by !**/build/**
• scripts/build/wifi_credentials.example.json is excluded by !**/build/**

📒 Files selected for processing (52)

• .github/workflows/release.yml
• .gitignore
• README.md
• docs/architecture.md
• docs/building.md
• docs/install/README.md
• docs/install/index.html
• docs/moonmodules/core/FirmwareUpdateModule.md
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/core/SystemModule.md
• docs/performance.md
• docs/plan.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• esp32/main/CMakeLists.txt
• scripts/MoonDeck.md
• scripts/check/collect_kpi.py
• scripts/docs/_test_metadata.py
• scripts/docs/generate_test_docs.py
• scripts/docs/screenshot_modules.py
• scripts/moondeck.py
• scripts/moondeck_config.json
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/index.html
• scripts/moondeck_ui/style.css
• scripts/scenario/_observed.py
• scripts/scenario/run_live_scenario.py
• scripts/scenario/run_scenario.py
• src/core/NetworkModule.h
• src/core/SystemModule.h
• src/light/layers/Layer.h
• src/main.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32.cpp
• src/ui/app.js
• src/ui/release-picker.js
• test/scenario_runner.cpp
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/core/scenario_NetworkModule_mdns_toggle.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_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/scenarios/light/scenario_PreviewDriver_detail.json
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_SystemModule.cpp
• test/unit/light/unit_ArtNetSendDriver_no_alloc_in_loop.cpp
• test/unit/light/unit_Layers_container.cpp

[coderabbitai]

Actionable comments posted: 15

Caution

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

⚠️ Outside diff range comments (3)

src/core/JsonSink.h (1)

87-99: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Honor fixed-buffer no-heap contract in appendf()

Line 88 allocates heap even in fixed-buffer mode, which breaks the mode contract and can hide capacity issues. Handle fixed mode before heap fallback.

💡 Proposed fix

-        // Fragment longer than the stack buffer — format into an exact heap buffer.
-        char* big = static_cast<char*>(platform::alloc(static_cast<size_t>(n) + 1));
-        if (big) {
-            std::vsnprintf(big, static_cast<size_t>(n) + 1, fmt, ap2);
-            append(big);
-            platform::free(big);
-        } else if (fixed_) {
-            // In fixed-buffer mode the caller checks overflowed() to decide
-            // whether the write completed; a silently-dropped fragment would
-            // look like a clean write. Flag the failure so the caller can
-            // abort the file/response.
-            overflowed_ = true;
-        }
+        if (fixed_) {
+            const size_t needed = static_cast<size_t>(n);
+            if (fixedLen_ + needed + 1 > fixedCap_) {
+                overflowed_ = true;
+            } else {
+                std::vsnprintf(fixed_ + fixedLen_, fixedCap_ - fixedLen_, fmt, ap2);
+                fixedLen_ += needed;
+            }
+            va_end(ap2);
+            return;
+        }
+        // Fragment longer than the stack buffer — format into an exact heap buffer.
+        char* big = static_cast<char*>(platform::alloc(static_cast<size_t>(n) + 1));
+        if (big) {
+            std::vsnprintf(big, static_cast<size_t>(n) + 1, fmt, ap2);
+            append(big);
+            platform::free(big);
+        }

🤖 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` around lines 87 - 99, The heap allocation in appendf()
occurs even when fixed_ mode is active; change the logic in the block that
handles a formatted fragment of length n so that you check fixed_ first and do
not call platform::alloc when fixed_ is true — instead set overflowed_ = true
(so the caller can abort) and skip append/alloc/free; only if fixed_ is false
should you allocate with platform::alloc, vsnprintf into the buffer, call
append(big), and then platform::free(big). Ensure you reference
appendf()/fixed_/overflowed_/platform::alloc/platform::free/append() when making
the change.

src/ui/release-picker.js (1)

30-35: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove roadmap wording from in-code comments.

This block still uses roadmap/future-tense language (“on the 2.0 roadmap”), which conflicts with the repo’s present-tense documentation/comment rule.

Based on learnings: “Use present tense only in code, comments, and documentation — no changelogs or roadmaps; history lives in git commits, except docs/plan.md and docs/history/.”

🤖 Prompt for AI Agents

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

In `@src/ui/release-picker.js` around lines 30 - 35, The comment block mentions
future roadmap language ("on the 2.0 roadmap") and should be rewritten in
present tense: remove the roadmap/future-tense phrase and state the current fact
that there is no JS unit-test harness and that the helpers (isCompatible,
parseFirmwaresFromAssets, relativeTime) have been exercised ad-hoc against real
GitHub Releases and synthesized assets; keep the existing reference to the C++
frame parser (ImprovFrame.h / test_improv_frame.cpp) but phrase it in present
tense rather than as future work.

scripts/moondeck_ui/app.js (1)

593-613: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid stacking duplicate change handlers in refreshPorts().

refreshPorts() is invoked multiple times, and each call adds another listener to #port-select. That causes duplicated saveState() requests per single port change.

💡 Proposed fix

 async function refreshPorts() {
@@
-    select.addEventListener("change", async () => {
+    select.onchange = async () => {
         const active = getActiveNetwork();
         if (active) active.port = select.value;
         await saveState();
-    });
+    };
 }

🤖 Prompt for AI Agents

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

In `@scripts/moondeck_ui/app.js` around lines 593 - 613, The refreshPorts function
is adding a new "change" listener to the `#port-select` element every time it
runs, which causes duplicate saveState calls; fix by ensuring only a single
handler is attached: locate refreshPorts and replace the addEventListener
approach with a non-duplicating pattern (e.g., clear existing listeners by
setting select.onchange = null then assign select.onchange = async () => { const
active = getActiveNetwork(); if (active) active.port = select.value; await
saveState(); } or clone/replace the node before adding the listener) so that
getActiveNetwork and saveState are invoked exactly once per user change.

♻️ Duplicate comments (3)

test/unit/core/unit_platform_clock.cpp (1)

19-27: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove timing-dependent uptime assertion on restore path.

Line 26 (CHECK(a > 0)) makes this test dependent on real process uptime; the restore contract is already covered by checking override values are no longer returned.

Proposed fix

     uint32_t a = mm::platform::millis();
     uint32_t b = mm::platform::millis();
     CHECK(a != 12345);
     CHECK(b != 67890);
-    CHECK(a > 0);

As per coding guidelines, `test/**`: “Tests should not depend on timing or network.”

🤖 Prompt for AI Agents

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

In `@test/unit/core/unit_platform_clock.cpp` around lines 19 - 27, The test
contains a timing-dependent assertion: after calling
mm::platform::setTestNowMs(0) and reading uint32_t a = mm::platform::millis();
the CHECK(a > 0) makes the test depend on real process uptime; remove that
assertion and keep only the restore-verification checks (the existing CHECK(a !=
12345) and CHECK(b != 67890)) so the test no longer relies on real time. Locate
the block using mm::platform::setTestNowMs(0), mm::platform::millis(), and
variables a/b and delete the CHECK(a > 0) line.

src/light/drivers/Correction.h (1)

54-57: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clamp invalid presets before the switch and drop default for exhaustiveness visibility.

Current default keeps runtime fallback, but it also hides future missing LightPreset cases. Clamp once, then switch exhaustively.

Suggested patch

     void rebuild(uint8_t brightness, LightPreset preset) {
+        if (static_cast<uint8_t>(preset) >= kLightPresetCount) {
+            preset = LightPreset::RGB;
+        }
         for (int v = 0; v < 256; v++) {
             briLut[v] = static_cast<uint8_t>((v * brightness) / 255);
         }
@@
-            // Out-of-range preset (e.g. a corrupt persisted lightPreset value cast to the
-            // enum) falls back to plain RGB so order/outChannels/deriveWhite are always set
-            // deterministically — apply() never reads stale fields.
-            default:                order[0]=0; order[1]=1; order[2]=2; outChannels=3; deriveWhite=false; break;
         }
     }

#!/bin/bash
set -euo pipefail
# Verify current warning flags and switch shape.
rg -n --hidden --no-ignore-vcs 'add_compile_options\(|-Wswitch-enum|-Wswitch|-Wall|-Wextra|-Werror' CMakeLists.txt test/CMakeLists.txt
nl -ba src/light/drivers/Correction.h | sed -n '35,85p'

As per coding guidelines: CMakeLists.txt must use strict warnings (-Wall -Wextra -Werror) with no exceptions.

🤖 Prompt for AI Agents

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

In `@src/light/drivers/Correction.h` around lines 54 - 57, Clamp the incoming
LightPreset value to the valid enum range before the switch (e.g. normalize or
bound the variable used in the switch), then remove the default: case so the
switch over LightPreset is exhaustive and the compiler can warn on missing enum
values; ensure the pre-switch clamp sets any out-of-range presets to a known
safe enum (so order, outChannels, deriveWhite get deterministic initialization)
and keep apply()'s assumptions intact. Reference the switch on LightPreset, the
variables order/outChannels/deriveWhite, and the apply() path when making the
change.

test/scenarios/light/scenario_GridLayout_resize.json (1)

61-67: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep this resize scenario deterministic by removing network/timing coupling.

This scenario currently validates resize behavior through a real network driver plus timing/FPS envelopes, which makes it flaky across targets and CI environments. Keep resize/reallocation assertions here, and move performance/network measurements to a separate non-gating benchmark scenario.

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

Also applies to: 100-395

🤖 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/light/scenario_GridLayout_resize.json` around lines 61 - 67,
This scenario is flaky because it exercises real networking/timing via the
"fix-artnet" module (id "ArtNet", type "ArtNetSendDriver") and timing/FPS
envelopes; replace that real network driver with a deterministic stub/no-op
driver (or mock driver) and remove any FPS/timing envelopes from this JSON so
resize/reallocation assertions remain stable, and move the performance/network
measurements into a separate benchmark scenario that uses the real
"ArtNetSendDriver".

🤖 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/coding-standards.md`:
- Line 42: The docs link for Correction.h is pointing to the wrong location;
update the markdown link that references "LightPreset + rebuild()" so it points
to the actual header at ../src/light/drivers/Correction.h instead of
../src/light/Correction.h (the symbols to locate are LightPreset and rebuild()
in Correction.h).

In `@docs/install/index.html`:
- Around line 306-309: The user-facing erase precondition text is inaccurate
because the variable lastInstallManifest is only assigned inside the onInstall
handler, so merely selecting a release/firmware doesn’t enable erase; update the
messaging near where the text is shown (the UI text that references "Pick a
release + firmware above first") to reflect the actual requirement — e.g.,
instruct the user to click the Install button or initiate an install (reference
lastInstallManifest and onInstall) before erase becomes available, and ensure
any similar copy near lines 323–327 is changed consistently.

In `@docs/moonmodules/core/ui.md`:
- Around line 117-120: Update the feature-summary count text that currently
reads “All 9 control types” to reflect the new total (now 11) so it matches the
control-type table which includes the added `display-int` and `ipv4` entries
(also check and update any other occurrences of that sentence in the same
document to keep them consistent with the table).

In `@docs/testing.md`:
- Around line 256-257: The documentation currently states that
observed[<target>] "carries the most-recent reading," which conflicts with the
new widen-only rolling range model; update the sentence referencing
observed[<target>] so it describes that it records a rolling [min,max] range
with associated timestamps (i.e., a widen-only persistent observation), rather
than a single most-recent reading, and ensure any related mention in the same
paragraph (around the `observed[<target>]` and "Persistent observations"
reference) reflects this range-with-timestamps behavior.

In `@scripts/docs/screenshot_modules.py`:
- Around line 533-537: The orphan-sweep blocks call delete_module(n) but only
catch exceptions, ignoring delete_module's False return on HTTP failure; update
both sweep paths (the try/except around delete_module in _sweep_orphans and the
similar block at the other sweep location) to check the return value and treat
False as a failure—log an error (or raise) when delete_module(...) returns
False, not just on exceptions, so failed deletes aren't silently ignored.

In `@scripts/moondeck.py`:
- Around line 336-349: save_state currently writes STATE_FILE directly and can
be corrupted by concurrent handlers; make writes atomic and thread-safe by
adding a module-level threading.Lock (e.g., state_write_lock) and using it in
save_state, and perform an atomic replace: serialize persisted to a temporary
file in the same directory (use tempfile.NamedTemporaryFile or mkstemp), flush
and fsync the file descriptor, close it, then os.replace(temp_path, STATE_FILE)
while holding state_write_lock; keep the existing _strip_network_volatiles call
and ensure exceptions during write still release the lock.

In `@scripts/scenario/run_live_scenario.py`:
- Around line 344-351: The code currently swallows exceptions from
collect_metrics and treats the measurement as skipped; change this to fail the
scenario so broken measure steps don't produce false positives: in the except
block for collect_metrics (referencing collect_metrics, step_name, step_result,
and results) set an explicit failure status (e.g., results["passed"]=False or
results["status"]="failed"), populate step_result with the error info, append it
to results["steps"], and then either raise the exception or return/exit so the
scenario run stops instead of continuing; ensure any downstream logic that
expects results["passed"] is updated accordingly.
- Around line 488-491: The loop that copies keys from existing into new_block
incorrectly carries forward a stale "max_alloc_block"; update the logic so when
handling the "max_alloc_block" key you refresh it from the current measured
value when the contract is opted-in instead of blindly copying existing[k]. In
other words, inside the loop over ("tick_tolerance_pct", "heap_tolerance_pct",
"tolerance_us", "max_alloc_block") keep the existing copy behavior for the first
three keys but for "max_alloc_block" check the contract-opted-in condition used
elsewhere in this module (same condition used by run_scenario.py) and assign
new_block["max_alloc_block"] from the current measurement/variable that holds
the fresh max_alloc_block value rather than from existing["max_alloc_block"].

In `@src/core/Control.cpp`:
- Around line 168-175: The Uint16 case currently always clamps via clampInto,
ignoring ApplyPolicy::Strict; update the ControlType::Uint16 branch in the same
switch (the one using mm::json::parseInt and clampInto) to first check the
control's policy (ApplyPolicy::Strict) and if strict and the parsed int v is
outside [0, UINT16_MAX] return the appropriate OutOfRange error/result instead
of clamping; otherwise preserve the existing behavior (use
clampInto/static_cast<uint16_t*> and return Ok). Ensure you use the same
OutOfRange/result type used by other control branches for consistency.

In `@src/core/JsonSink.h`:
- Around line 117-140: The writeJsonString method in JsonSink.h currently only
escapes '"' and '\\' and writes raw control bytes (e.g., '\n','\r','\t' and any
byte < 0x20), producing invalid JSON; update writeJsonString to detect and emit
proper JSON escapes for control characters (replace '\n' with "\\n", '\r' with
"\\r", '\t' with "\\t", '\b' with "\\b", '\f' with "\\f') and for any other byte
< 0x20 emit a Unicode escape sequence "\\u00XX" (hex-encode the byte) instead of
writing the raw byte; keep using the existing append(...) strategy and the local
buffer (one) pattern but branch on control cases and construct the appropriate
escape strings before calling append so all low-valued bytes are safely escaped.

In `@src/light/drivers/ArtNetSendDriver.h`:
- Around line 92-101: The code only checks corrected_.count() >= nLights before
applying correction but doesn't verify the per-light channel stride, so dst + i
* outCh may overflow if correction_->outChannels > corrected_ stride; update the
guard to ensure corrected_ can hold nLights * outCh (e.g. check
corrected_.count() >= nLights AND that corrected_ has at least
correction_->outChannels per light or total bytes >= nLights * outCh), and if
not, call resizeCorrected(nLights, correction_->outChannels) (or an equivalent
resize) before running the loop; reference correction_,
correction_->outChannels, corrected_.data(), corrected_.count(), and
resizeCorrected() when making this change.

In `@src/light/layers/Layers.h`:
- Around line 36-39: The Layers container must restrict loop propagation to
children with ModuleRole::Layer to avoid executing stray non-Layer children;
restore or add a filter in Layers::loop() so it iterates only over child modules
whose role equals ModuleRole::Layer (consistent with setLayouts() and
activeLayer()), and call each child's loop/tick only when enabled and
role==ModuleRole::Layer, leaving MoonModule::loop() behavior unchanged.

In `@test/scenario_runner.cpp`:
- Around line 487-497: The measurement branch currently only runs when step has
"measure": true, but earlier you treat op == "measure" as implicit measurement;
update the measurement trigger so op:"measure" actually executes the measurement
flow. Specifically, change the condition that starts measurement (the if that
checks step.has("measure") && step["measure"].boolean) to also accept
std::strcmp(op, "measure") == 0 (or set a local bool like isMeasure =
(step.has("measure") && step["measure"].boolean) || std::strcmp(op,"measure")==0
and use it), so ensureStarted() and the per-step measurement logic run for
op:"measure" steps.

In `@test/scenarios/light/scenario_Layer_memory_1to1.json`:
- Around line 60-99: The scenario currently adds a network-dependent,
timing-sensitive module ("add-artnet" of type ArtNetSendDriver) and includes
measurement/config keys ("measure", "bounds", "contract", "observed"); remove
the network/timing coupling by replacing or removing that module entry: either
remove the "add-artnet" block entirely or change "type": "ArtNetSendDriver" to a
deterministic no-op/local stub driver and delete the "measure", "bounds",
"contract", and "observed" fields (or set them to neutral defaults) so the test
no longer depends on network I/O or host timing; ensure references to "id":
"ArtNet" and "parent_id": "Drivers" remain consistent if you keep a stub module.

In `@test/unit/light/unit_FireEffect.cpp`:
- Around line 77-79: The test currently calls fire.onBuildState() directly after
fire.setEnabled(false); replace that direct call with invoking the parent
layer's lifecycle so the disable path is exercised the same as production: after
fire.setEnabled(false) call Layer::onBuildState() on the parent object
(referenced as layer) instead of fire.onBuildState(), then assert
fire.dynamicBytes() == 0; this ensures the rebuild is triggered via the layer
lifecycle rather than bypassing it.

---

Outside diff comments:
In `@scripts/moondeck_ui/app.js`:
- Around line 593-613: The refreshPorts function is adding a new "change"
listener to the `#port-select` element every time it runs, which causes duplicate
saveState calls; fix by ensuring only a single handler is attached: locate
refreshPorts and replace the addEventListener approach with a non-duplicating
pattern (e.g., clear existing listeners by setting select.onchange = null then
assign select.onchange = async () => { const active = getActiveNetwork(); if
(active) active.port = select.value; await saveState(); } or clone/replace the
node before adding the listener) so that getActiveNetwork and saveState are
invoked exactly once per user change.

In `@src/core/JsonSink.h`:
- Around line 87-99: The heap allocation in appendf() occurs even when fixed_
mode is active; change the logic in the block that handles a formatted fragment
of length n so that you check fixed_ first and do not call platform::alloc when
fixed_ is true — instead set overflowed_ = true (so the caller can abort) and
skip append/alloc/free; only if fixed_ is false should you allocate with
platform::alloc, vsnprintf into the buffer, call append(big), and then
platform::free(big). Ensure you reference
appendf()/fixed_/overflowed_/platform::alloc/platform::free/append() when making
the change.

In `@src/ui/release-picker.js`:
- Around line 30-35: The comment block mentions future roadmap language ("on the
2.0 roadmap") and should be rewritten in present tense: remove the
roadmap/future-tense phrase and state the current fact that there is no JS
unit-test harness and that the helpers (isCompatible, parseFirmwaresFromAssets,
relativeTime) have been exercised ad-hoc against real GitHub Releases and
synthesized assets; keep the existing reference to the C++ frame parser
(ImprovFrame.h / test_improv_frame.cpp) but phrase it in present tense rather
than as future work.

---

Duplicate comments:
In `@src/light/drivers/Correction.h`:
- Around line 54-57: Clamp the incoming LightPreset value to the valid enum
range before the switch (e.g. normalize or bound the variable used in the
switch), then remove the default: case so the switch over LightPreset is
exhaustive and the compiler can warn on missing enum values; ensure the
pre-switch clamp sets any out-of-range presets to a known safe enum (so order,
outChannels, deriveWhite get deterministic initialization) and keep apply()'s
assumptions intact. Reference the switch on LightPreset, the variables
order/outChannels/deriveWhite, and the apply() path when making the change.

In `@test/scenarios/light/scenario_GridLayout_resize.json`:
- Around line 61-67: This scenario is flaky because it exercises real
networking/timing via the "fix-artnet" module (id "ArtNet", type
"ArtNetSendDriver") and timing/FPS envelopes; replace that real network driver
with a deterministic stub/no-op driver (or mock driver) and remove any
FPS/timing envelopes from this JSON so resize/reallocation assertions remain
stable, and move the performance/network measurements into a separate benchmark
scenario that uses the real "ArtNetSendDriver".

In `@test/unit/core/unit_platform_clock.cpp`:
- Around line 19-27: The test contains a timing-dependent assertion: after
calling mm::platform::setTestNowMs(0) and reading uint32_t a =
mm::platform::millis(); the CHECK(a > 0) makes the test depend on real process
uptime; remove that assertion and keep only the restore-verification checks (the
existing CHECK(a != 12345) and CHECK(b != 67890)) so the test no longer relies
on real time. Locate the block using mm::platform::setTestNowMs(0),
mm::platform::millis(), and variables a/b and delete the CHECK(a > 0) line.

🪄 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: 3e4fb7b6-aa3d-4bcb-adeb-f6783ab63463

📥 Commits

Reviewing files that changed from the base of the PR and between 6281ca5 and 553dad2.

⛔ Files ignored due to path filters (10)

• docs/assets/screenshots/moondeck_esp32.png is excluded by !**/*.png
• docs/assets/screenshots/moondeck_live.png is excluded by !**/*.png
• docs/assets/screenshots/moondeck_pc.png is excluded by !**/*.png
• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/clean_esp32.py is excluded by !**/build/**
• scripts/build/flash_esp32.py is excluded by !**/build/**
• scripts/build/generate_build_info.py is excluded by !**/build/**
• scripts/build/generate_manifest.py is excluded by !**/build/**
• scripts/build/host_wifi.py is excluded by !**/build/**
• scripts/build/wifi_credentials.example.json is excluded by !**/build/**

📒 Files selected for processing (149)

• .github/workflows/release.yml
• .gitignore
• CLAUDE.md
• CMakeLists.txt
• README.md
• docs/architecture.md
• docs/building.md
• docs/coding-standards.md
• docs/history/decisions.md
• docs/install/README.md
• docs/install/index.html
• docs/moonmodules/core/Control.md
• docs/moonmodules/core/FilesystemModule.md
• docs/moonmodules/core/FirmwareUpdateModule.md
• docs/moonmodules/core/HttpServerModule.md
• docs/moonmodules/core/ImprovProvisioningModule.md
• docs/moonmodules/core/MoonModule.md
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/core/SystemModule.md
• docs/moonmodules/core/ui.md
• docs/moonmodules/light/Buffer.md
• docs/moonmodules/light/Drivers.md
• docs/moonmodules/light/EffectBase.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/Layouts.md
• docs/moonmodules/light/LightConfig.md
• docs/moonmodules/light/drivers/ArtNetSendDriver.md
• docs/moonmodules/light/drivers/PreviewDriver.md
• docs/moonmodules/light/effects/CheckerboardEffect.md
• docs/moonmodules/light/effects/FireEffect.md
• docs/moonmodules/light/effects/GlowParticlesEffect.md
• docs/moonmodules/light/effects/LavaLampEffect.md
• docs/moonmodules/light/effects/MetaballsEffect.md
• docs/moonmodules/light/effects/NoiseEffect.md
• docs/moonmodules/light/effects/ParticlesEffect.md
• docs/moonmodules/light/effects/PlasmaEffect.md
• docs/moonmodules/light/effects/PlasmaPaletteEffect.md
• docs/moonmodules/light/effects/RainbowEffect.md
• docs/moonmodules/light/effects/RipplesEffect.md
• docs/moonmodules/light/effects/SpiralEffect.md
• docs/moonmodules/light/layouts/GridLayout.md
• docs/moonmodules/light/leddriver-analysis-bottom-up.md
• docs/moonmodules/light/leddriver-analysis-top-down.md
• docs/moonmodules/light/modifiers/MirrorModifier.md
• docs/performance.md
• docs/plan.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• esp32/main/CMakeLists.txt
• scripts/MoonDeck.md
• scripts/check/collect_kpi.py
• scripts/docs/_test_metadata.py
• scripts/docs/generate_test_docs.py
• scripts/docs/screenshot_modules.py
• scripts/moondeck.py
• scripts/moondeck_config.json
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/index.html
• scripts/moondeck_ui/style.css
• scripts/scenario/_observed.py
• scripts/scenario/run_live_scenario.py
• scripts/scenario/run_scenario.py
• scripts/test/test_desktop.py
• src/core/Control.cpp
• src/core/Control.h
• src/core/FilesystemModule.cpp
• src/core/FilesystemModule.h
• src/core/HttpServerModule.cpp
• src/core/JsonSink.h
• src/core/MoonModule.h
• src/core/NetworkModule.h
• src/core/Scheduler.cpp
• src/core/Scheduler.h
• src/core/SystemModule.h
• src/light/drivers/ArtNetSendDriver.h
• src/light/drivers/Correction.h
• src/light/drivers/Drivers.h
• src/light/drivers/PreviewDriver.h
• src/light/effects/FireEffect.h
• src/light/effects/ParticlesEffect.h
• src/light/layers/Layer.h
• src/light/layers/Layers.h
• src/light/layouts/Layouts.h
• src/light/modifiers/ModifierBase.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_config.h
• src/platform/esp32/platform_esp32.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/release-picker.js
• test/CMakeLists.txt
• test/scenario_runner.cpp
• test/scenarios/base-pipeline.json
• test/scenarios/control-change.json
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/core/scenario_NetworkModule_mdns_toggle.json
• test/scenarios/grid-resize.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_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/scenarios/light/scenario_PreviewDriver_detail.json
• test/scenarios/mdns-toggle.json
• test/scenarios/memory-1to1.json
• test/scenarios/memory-lut.json
• test/scenarios/mirror.json
• test/scenarios/preview-detail.json
• test/test_network_module.cpp
• test/unit/core/unit_Buffer.cpp
• test/unit/core/unit_Color.cpp
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_ImprovFrame.cpp
• test/unit/core/unit_MappingLUT.cpp
• test/unit/core/unit_ModuleFactory.cpp
• test/unit/core/unit_MoonModule.cpp
• test/unit/core/unit_MoonModule_control_change_gate.cpp
• test/unit/core/unit_MoonModule_lifecycle.cpp
• test/unit/core/unit_MoonModule_movechild.cpp
• test/unit/core/unit_MoonModule_replacechild.cpp
• test/unit/core/unit_NetworkModule.cpp
• test/unit/core/unit_Scheduler_unique_names.cpp
• test/unit/core/unit_SystemModule.cpp
• test/unit/core/unit_platform_clock.cpp
• test/unit/light/unit_ArtNetSendDriver_no_alloc_in_loop.cpp
• test/unit/light/unit_ArtNetSendDriver_packet.cpp
• test/unit/light/unit_BlendMap.cpp
• test/unit/light/unit_CheckerboardEffect.cpp
• test/unit/light/unit_Correction.cpp
• test/unit/light/unit_Drivers_container.cpp
• test/unit/light/unit_FireEffect.cpp
• test/unit/light/unit_GridLayout.cpp
• test/unit/light/unit_Layer_extrude.cpp
• test/unit/light/unit_Layer_phase_animation.cpp
• test/unit/light/unit_Layer_zero_grid.cpp
• test/unit/light/unit_Layers_container.cpp
• test/unit/light/unit_Layouts_container.cpp
• test/unit/light/unit_Layouts_toggle_cycle.cpp
• test/unit/light/unit_MetaballsEffect.cpp
• test/unit/light/unit_MirrorModifier.cpp
• test/unit/light/unit_NoiseEffect.cpp
• test/unit/light/unit_ParticlesEffect.cpp
• test/unit/light/unit_PlasmaEffect.cpp
• test/unit/light/unit_PreviewDriver.cpp
• test/unit/light/unit_RainbowEffect.cpp

💤 Files with no reviewable changes (9)

• test/scenarios/preview-detail.json
• test/scenarios/control-change.json
• test/scenarios/base-pipeline.json
• test/scenarios/mdns-toggle.json
• test/test_network_module.cpp
• test/scenarios/grid-resize.json
• test/scenarios/memory-1to1.json
• test/scenarios/mirror.json
• test/scenarios/memory-lut.json

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove network/timing coupling from this scenario.

This scenario depends on a network driver (ArtNetSendDriver) and hard performance/timing assertions (measure, bounds, contract, observed), which makes normal test execution host- and environment-sensitive.

Suggested minimal direction

-      "name": "add-artnet",
+      "name": "add-preview",
       "op": "add_module",
-      "id": "ArtNet",
-      "type": "ArtNetSendDriver",
+      "id": "Preview",
+      "type": "PreviewDriver",
       "parent_id": "Drivers",
-      "measure": true,
-      "bounds": { ... },
-      "contract": { ... },
-      "observed": { ... }
+      "props": {
+        "fps": 24
+      }

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

🤖 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/light/scenario_Layer_memory_1to1.json` around lines 60 - 99,
The scenario currently adds a network-dependent, timing-sensitive module
("add-artnet" of type ArtNetSendDriver) and includes measurement/config keys
("measure", "bounds", "contract", "observed"); remove the network/timing
coupling by replacing or removing that module entry: either remove the
"add-artnet" block entirely or change "type": "ArtNetSendDriver" to a
deterministic no-op/local stub driver and delete the "measure", "bounds",
"contract", and "observed" fields (or set them to neutral defaults) so the test
no longer depends on network I/O or host timing; ensure references to "id":
"ArtNet" and "parent_id": "Drivers" remain consistent if you keep a stub module.

[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 (4)

scripts/scenario/run_live_scenario.py (1)

248-259: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast when a reset step cannot be applied.

The reset block defines the baseline for the whole scenario, but a /api/control failure here only logs WARN and execution continues into baseline collection. That can persist observed data and renegotiate contracts from the wrong starting state.

Suggested fix

             except Exception as e:
-                print(f"  WARN  reset {r_step.get('name','?')}: {e}")
+                print(f"  FAIL  reset {r_step.get('name','?')}: {e}")
+                results["passed"] = False
+                results["error"] = (
+                    f"reset failed for {r_step.get('id','?')}.{r_step.get('key','?')}: {e}"
+                )
+                return results

🤖 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 248 - 259, The reset loop
currently swallows failures from client.post("/api/control") and proceeds to
call collect_metrics, which can record metrics from a wrong state; change the
except block around the client.post call that handles r_step to fail-fast:
replace the silent WARN-only behavior with an explicit abort (re-raise the
caught exception or call sys.exit(1)/raise RuntimeError) after logging so
execution stops before baseline = collect_metrics(...). Ensure you update the
try/except that references r_step and client.post("/api/control") so any control
error prevents proceeding to collect_metrics.

src/core/NetworkModule.h (1)

102-147: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Call MoonModule::onBuildControls() before appending NetworkModule controls.

The new lifecycle contract in docs/coding-standards.md makes onBuildControls() children-first by default. Keeping the base call at the end leaves NetworkModule out of contract and changes child-control ordering without an exception note.

♻️ Minimal reorder

     void onBuildControls() override {
+        MoonModule::onBuildControls();
+
         setStatus(statusBuf_);
 
         // Refresh the live-readout values (mode label + rssi + txPower) so a
         // rebuild triggered mid-state-transition shows the up-to-date numbers.
         updateMetrics();
@@
-        // Chain to base so children (Improv on ESP32) get their controls built too.
-        MoonModule::onBuildControls();
     }

As per coding guidelines, `onBuildControls` should “chain first, then parent work. Children register their controls before the parent appends or rewires its own.”

🤖 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/NetworkModule.h` around lines 102 - 147, Move the base-class
chaining call so child modules build their controls first: in
NetworkModule::onBuildControls(), call MoonModule::onBuildControls() at the
start of the method (before invoking setStatus, updateMetrics, or adding
controls via controls_.add*/controls_.setHidden) rather than at the end, so the
child-first lifecycle contract is honored and NetworkModule then appends/rewires
its controls.

test/scenarios/light/scenario_Layer_base_pipeline.json (1)

71-96: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Latest pc-macos observation fails this scenario's own FPS contract.

contract.pc-macos.tick_us = 50 implies a 20,000 FPS baseline, so the min_pct: 80 floor caps the measured tick at 62.5µs. The new 132µs observation on Line 83 is only about 7,576 FPS, so this fixture now records a failing run as the latest result. Please either refresh the observation from a passing run or update the contract/bounds to the new reality before merge.

Based on learnings: "Tests must pass. Run ctest (unit tests) and ./build/test/mm_scenarios (scenarios) before considering work complete."

🤖 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/light/scenario_Layer_base_pipeline.json` around lines 71 - 96,
The latest observed value for pc-macos tick_us (observed.pc-macos.tick_us
contains [35, 132]) violates the scenario contract (contract.pc-macos.tick_us =
50) so either refresh the failing observation to a value within the contract
floor or loosen the contract/bounds to reflect current reality; specifically
update observed.pc-macos.tick_us to a passing measurement (e.g., <=62.5) or
adjust contract.pc-macos.tick_us or any min_pct floor so the 132µs sample is
acceptable, then run ctest and ./build/test/mm_scenarios to verify all scenario
tests pass before merge.

test/scenarios/light/scenario_MirrorModifier_pipeline.json (1)

77-101: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Latest pc-macos observation is below this step's published contract.

With contract.pc-macos.tick_us = 120, the 80% floor here is 150µs. The new 213µs observation on Lines 87-89 is only about 4,695 FPS, so this scenario now records a failing latest run while still advertising the old bound. Please refresh the measurement from a passing run or move the contract/bounds to the new baseline.

Based on learnings: "Tests must pass. Run ctest (unit tests) and ./build/test/mm_scenarios (scenarios) before considering work complete."

🤖 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/light/scenario_MirrorModifier_pipeline.json` around lines 77 -
101, The latest observed pc-macos tick_us (213) exceeds the published contract
tick_us (120), so update the scenario to make tests consistent: either rerun the
scenario on a passing run and replace observed.pc-macos.tick_us with
measurements that satisfy the existing contract, or move the contract baseline
by setting contract.pc-macos.tick_us to a new value at or above the latest
passing measurement (e.g., >=213) and adjust any derived bounds (the 80% floor)
accordingly; after making the change, run ctest and ./build/test/mm_scenarios to
verify the scenario passes.

♻️ Duplicate comments (2)

test/scenario_runner.cpp (1)

509-519: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

min_pct performance budgets are still not enforced in mm_scenarios.

This branch warns instead of collapsing to fps > 0, but the effect is unchanged: any step that declares only bounds.fps.min_pct still has no in-process assertion. That leaves scenarios like scenario_PreviewDriver_detail.json green even when the local regression budget is missed. Either compute the bound from an in-run baseline or keep these steps out of the in-process contract until the runner can honor them. Based on learnings: full pipeline needs a scenario test.

🤖 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 509 - 519, The current branch logs and
sets fpsBound=0 when encountering step["bounds"]["fps"]["min_pct"], which leaves
min_pct unenforced; instead compute an in-process bound from a local baseline
when possible: read the declared min_pct (step["bounds"]["fps"]["min_pct"].num)
and compute fpsBound = ceil(localBaselineFps * (min_pct/100.0)) using whatever
in-process baseline metric is already collected (e.g., the existing
baseline-storage or measurement API used elsewhere in scenario_runner.cpp), and
only fall back to the current warning/skip if no local baseline measurement
exists; update references to fpsBound and the warning message (which includes
name) accordingly so steps like scenario_PreviewDriver_detail.json get an
enforced assertion.

test/scenarios/light/scenario_GridLayout_resize.json (1)

61-67: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep this scenario off timing/network paths.

This still exercises a real ArtNetSendDriver and asserts timing/heap envelopes, so it remains target-dependent and flaky in CI. Split the resize/reallocation check into a deterministic scenario, and move ArtNet/perf sampling to a separate non-gating benchmark flow.

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

Also applies to: 100-395

🤖 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/light/scenario_GridLayout_resize.json` around lines 61 - 67,
The scenario includes a real ArtNet driver ("fix-artnet" module with id "ArtNet"
and type "ArtNetSendDriver") which makes the test timing- and network-dependent;
remove or relocate that module from the gating scenario_GridLayout_resize.json
and instead create two artifacts: a deterministic unit scenario that only
exercises the grid resize/reallocation checks (no "ArtNetSendDriver" or
network/timing sampling), and a separate non-gating benchmark/perf scenario that
adds the "fix-artnet" / id "ArtNet" / type "ArtNetSendDriver" module for
timing/heap sampling; update references to "Drivers" parent as needed and ensure
the resize assertions live only in the deterministic scenario.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@scripts/moondeck.py`:
- Around line 337-384: The current lock in save_state only serializes the file
write but not the full load/mutate/save cycle, which allows concurrent handlers
to read the same snapshot and clobber each other; change this by introducing a
mutate_state(...) helper that acquires _state_write_lock, calls load_state(),
passes the mutable state into the caller-supplied mutator, then calls
save_state(updated_state) (or persists the mutated in-memory state) before
releasing the lock; update each mutating HTTP handler (the handlers for
/api/discover, /api/refresh and any handler that currently does load_state() →
mutate → save_state()) to call mutate_state() instead of doing the three-step
sequence themselves so the entire transaction is atomic and serialized.

In `@scripts/run/preview_installer.py`:
- Around line 201-209: The printed guidance currently tells users to pick
`local-dev` but the preview stages artifacts under LOCAL_TAG (set to "latest"),
causing a mismatch; update the message emitted after stage_local_builds(...)
(and any related prints) to reference the actual LOCAL_TAG (or the literal
"latest") instead of `local-dev` so the picker points to the correct tag; locate
find_local_builds, stage_local_builds and the print block and change the second
print from mentioning `local-dev` to use LOCAL_TAG/"latest".

In `@src/core/NetworkModule.h`:
- Around line 57-62: When wifiStaInit(ssid_, password_) returns true and you set
state_ = State::WaitingSta and update status, also trigger a controls rebuild so
the UI hides the rssi control immediately; call onBuildControls() (or the
module's controls-refresh API used elsewhere) right after setting state_ and
stateChangeTime_ to force re-evaluation of control visibility, and add a module
test exercising the ConnectedSta -> WaitingSta transition to assert the rssi
control is hidden.

In `@src/ui/app.js`:
- Around line 1144-1152: fmtDisplayInt currently only hides null/undefined but
the code/comments expect 0 to be treated as an "unavailable" sentinel; update
fmtDisplayInt to treat a zero sentinel as unavailable by returning an empty
string when ctrl.value === 0 (in addition to null/undefined). Locate function
fmtDisplayInt and change the early-return condition that checks v to also return
"" for v === 0 so the UI doesn't render bogus "0" or "0 dBm" values.
- Around line 1018-1035: The ipv4 case must mirror other mutable inputs: add a
focus listener that stamps dragTs[ctrl.name] = Date.now() to start the cooldown
and change the send path to the same blur/change flow used by other inputs so
server errors are surfaced (don’t let sendControl swallow failures); on
blur/change call the same logic that sends the control, inspects the response
and on non-OK shows the error and re-stamps dragTs[ctrl.name] to prevent an
intervening WS push from reverting the edit. Use the same event types and
handlers (focus/blur/change) and the existing sendControl and dragTs symbols so
the ipv4 control participates in the same stale-patch/error protection as the
other writable inputs.

In `@test/unit/core/unit_NetworkModule.cpp`:
- Around line 152-173: Count how many controls match "rssi" or "txPower" instead
of just checking when found; increment a matchCount while iterating
net.controls() and after the loop assert the expected count: for Wi‑Fi-capable
builds require matchCount == 2, and for Ethernet-only builds require matchCount
== 0. Detect build capability either via an existing runtime query on
mm::NetworkModule (e.g., a hypothetical net.hasWiFi() / supportsWiFi() method)
or via the compile-time flag your project uses for Wi‑Fi (e.g., a WIFI_SUPPORTED
macro), and use that to choose which assertion to make; keep the existing checks
for type and hidden inside the loop while incrementing matchCount.

---

Outside diff comments:
In `@scripts/scenario/run_live_scenario.py`:
- Around line 248-259: The reset loop currently swallows failures from
client.post("/api/control") and proceeds to call collect_metrics, which can
record metrics from a wrong state; change the except block around the
client.post call that handles r_step to fail-fast: replace the silent WARN-only
behavior with an explicit abort (re-raise the caught exception or call
sys.exit(1)/raise RuntimeError) after logging so execution stops before baseline
= collect_metrics(...). Ensure you update the try/except that references r_step
and client.post("/api/control") so any control error prevents proceeding to
collect_metrics.

In `@src/core/NetworkModule.h`:
- Around line 102-147: Move the base-class chaining call so child modules build
their controls first: in NetworkModule::onBuildControls(), call
MoonModule::onBuildControls() at the start of the method (before invoking
setStatus, updateMetrics, or adding controls via
controls_.add*/controls_.setHidden) rather than at the end, so the child-first
lifecycle contract is honored and NetworkModule then appends/rewires its
controls.

In `@test/scenarios/light/scenario_Layer_base_pipeline.json`:
- Around line 71-96: The latest observed value for pc-macos tick_us
(observed.pc-macos.tick_us contains [35, 132]) violates the scenario contract
(contract.pc-macos.tick_us = 50) so either refresh the failing observation to a
value within the contract floor or loosen the contract/bounds to reflect current
reality; specifically update observed.pc-macos.tick_us to a passing measurement
(e.g., <=62.5) or adjust contract.pc-macos.tick_us or any min_pct floor so the
132µs sample is acceptable, then run ctest and ./build/test/mm_scenarios to
verify all scenario tests pass before merge.

In `@test/scenarios/light/scenario_MirrorModifier_pipeline.json`:
- Around line 77-101: The latest observed pc-macos tick_us (213) exceeds the
published contract tick_us (120), so update the scenario to make tests
consistent: either rerun the scenario on a passing run and replace
observed.pc-macos.tick_us with measurements that satisfy the existing contract,
or move the contract baseline by setting contract.pc-macos.tick_us to a new
value at or above the latest passing measurement (e.g., >=213) and adjust any
derived bounds (the 80% floor) accordingly; after making the change, run ctest
and ./build/test/mm_scenarios to verify the scenario passes.

---

Duplicate comments:
In `@test/scenario_runner.cpp`:
- Around line 509-519: The current branch logs and sets fpsBound=0 when
encountering step["bounds"]["fps"]["min_pct"], which leaves min_pct unenforced;
instead compute an in-process bound from a local baseline when possible: read
the declared min_pct (step["bounds"]["fps"]["min_pct"].num) and compute fpsBound
= ceil(localBaselineFps * (min_pct/100.0)) using whatever in-process baseline
metric is already collected (e.g., the existing baseline-storage or measurement
API used elsewhere in scenario_runner.cpp), and only fall back to the current
warning/skip if no local baseline measurement exists; update references to
fpsBound and the warning message (which includes name) accordingly so steps like
scenario_PreviewDriver_detail.json get an enforced assertion.

In `@test/scenarios/light/scenario_GridLayout_resize.json`:
- Around line 61-67: The scenario includes a real ArtNet driver ("fix-artnet"
module with id "ArtNet" and type "ArtNetSendDriver") which makes the test
timing- and network-dependent; remove or relocate that module from the gating
scenario_GridLayout_resize.json and instead create two artifacts: a
deterministic unit scenario that only exercises the grid resize/reallocation
checks (no "ArtNetSendDriver" or network/timing sampling), and a separate
non-gating benchmark/perf scenario that adds the "fix-artnet" / id "ArtNet" /
type "ArtNetSendDriver" module for timing/heap sampling; update references to
"Drivers" parent as needed and ensure the resize assertions live only in the
deterministic scenario.

🪄 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: 8b1fcaa1-5146-4ee6-9042-0fb2e82afeff

📥 Commits

Reviewing files that changed from the base of the PR and between 302c5e6 and d07d26a.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (50)

• CLAUDE.md
• CMakeLists.txt
• docs/building.md
• docs/coding-standards.md
• docs/install/index.html
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/core/ui.md
• docs/moonmodules/light/drivers/ArtNetSendDriver.md
• docs/plan.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• esp32/main/CMakeLists.txt
• scripts/MoonDeck.md
• scripts/docs/generate_test_docs.py
• scripts/docs/screenshot_modules.py
• scripts/moondeck.py
• scripts/moondeck_config.json
• scripts/moondeck_ui/app.js
• scripts/moondeck_ui/style.css
• scripts/run/preview_installer.py
• scripts/scenario/run_live_scenario.py
• scripts/scenario/run_scenario.py
• src/core/Control.cpp
• src/core/Control.h
• src/core/FilesystemModule.cpp
• src/core/FilesystemModule.h
• src/core/HttpServerModule.cpp
• src/core/JsonSink.h
• src/core/NetworkModule.h
• src/light/drivers/ArtNetSendDriver.h
• src/light/drivers/Correction.h
• src/light/layers/Layers.h
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/release-picker.js
• test/scenario_runner.cpp
• test/scenarios/core/scenario_MoonModule_control_change.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_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/scenarios/light/scenario_PreviewDriver_detail.json
• test/unit/core/unit_NetworkModule.cpp
• test/unit/core/unit_platform_clock.cpp
• test/unit/light/unit_FireEffect.cpp

💤 Files with no reviewable changes (1)

• src/core/FilesystemModule.h