Multi-layer composition + RegionModifier + semver versioning/update badge

[ewowi]

Summary

A multi-feature branch on top of main: industry-standard semver versioning + an update-available badge, multi-layer composition (stack and blend layers), the RegionModifier (crop a layer to a percentage sub-rectangle), routing firmware OTA status through the shared module status slot, a CodeRabbit-findings pass, and a documentation de-duplication pass. Also slims the web installer page.

---

1. Multi-layer composition

The Layers container now composites more than one enabled Layer into the shared output. Each Layer renders into its own buffer; Drivers blends them bottom→top per each Layer's blendMode (alpha-over / additive) and opacity (the two new Layer controls that replaced the six inert start/end ones). A single enabled Layer stays the byte-for-byte pass-through fast path. Blend math is integer-only (BlendMap.h: 8-bit alpha-over, additive sum-with-clamp, the (x+(x>>8)+1)>>8 div-255), branch-resolved once per layer.

2. RegionModifier (region carving)

Cropping a layer to a sub-rectangle of the physical box is a modifier, not a Layer control: RegionModifier carries startX/Y/Z–endX/Y/Z percentages and shrinks the logical box to the region (effect renders there, rest dark). Bounds are half-open [start, end) so abutting layers tile exactly. Full coverage = no modifier = the existing identity fast path, zero added cost. Region and Multiply are independent and stack once modifier chaining lands.

3. Firmware OTA status via the shared status slot

OTA flash progress (idle/downloading/flashing/error: …) now flows through MoonModule::setStatus() — the same per-module status banner every module uses — instead of a bespoke update_status control. An error: prefix maps to Severity::Error, idle clears the banner.

4. Semver-clean version + per-build latest + update badge

(unchanged from the original PR scope.)

• Semver-clean version. The Firmware card's version is now pure semver (2.0.0, or 2.1.0-dev.<N> on a moving build); the channel is derivable from the prerelease suffix, not mixed in. library.json moves to 2.1.0-dev.
• Per-build latest version. Each latest build carries a monotonic 2.1.0-dev.<N> (N = commits since the last v* tag, via compute_version.py), threaded through MM_VERSION, asset names, and the manifest. semver.org §11 orders these.
• Update-available badge (two channels). Status-bar badge modelled on ESP32-sveltekit's UpdateIndicator (our own code, no npm dep). Stable devices compare against releases/latest; -dev devices also check the moving latest. Cached in localStorage (1 h TTL), forced-fresh when the Firmware card opens, in-flight de-duplicated, click pre-selects the release in the picker.
• Installer CSS/JS extraction. docs/install/index.html (2080 → 402 lines): inline <style>/<script> → install.css/install.js. A dependency-free src/ui/semver.js is the one home for version comparison.

5. CodeRabbit findings + doc cleanup

• release.yml: release tag passed via env (not inline ${{ }}) to both compute_version steps; version published as the release name.
• Dev-channel update check reads the version from the CORS-readable GitHub API instead of the redirect-blocked manifest asset.
• Drivers/Layers: output selection uses firstEnabledLayer() so an all-disabled-layer Layer goes idle instead of emitting a stale frame.
• CLAUDE.md: added "document a thing once, reference it generically"; de-duplication sweep across architecture/Layer/Control/unit-tests.

Testing

• Pre-commit gates pass on every commit: spec, desktop build (-Werror), ctest (472 cases), scenarios (13), platform boundary, ESP32 build, KPI, firmware-list, host JS + Python.
• New tests: unit_RegionModifier (region math + mapping), unit_BlendMap (the blend ops), unit_Layers_container (composition + firstEnabledLayer + drivers-idle-when-all-disabled), unit_Layer_sparse_mapping (carving integration), unit_FirmwareUpdateModule (status routing).
• Bench-verified on ESP32-S3 (semver/badge work): clean semver, injected 2.1.0-dev.<N>, stable badge appears + opens Firmware pre-selected, /semver.js served.

Release notes

• Bump library.json 2.1.0-dev → 2.1.0 before tagging the next stable.
• Multi-layer default is additive for stacking light; alpha-over at full opacity overwrites lower layers (no per-pixel transparency yet — true occlusion would need RGBA, tracked separately).
• Perf: the multi-layer composite + RegionModifier tick cost is not yet captured in docs/performance.md (needs a live bench run; the connected device was unavailable this session).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added a firmware update badge in the status bar that highlights available updates and opens the Firmware view.
  • Introduced region-based light cropping controls for effects, plus multi-layer light composition with blend modes and opacity.
  • Expanded the web installer with a richer standalone interface and device selection flow.

• Bug Fixes

  • Improved version handling so firmware versions compare correctly and update checks are more reliable.
  • Fixed status display behavior for firmware update progress and errors.

• Documentation

  • Updated versioning, installer, and light-system docs to match the new behavior.

• Tests

  • Added and updated tests for version parsing, update checks, layer blending, and region cropping.

[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: 0fdbd97d-984d-4e0f-a419-6cb91c8649d3

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:

• ✅ Review completed - (🔄 Check again to review again)

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[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 `@docs/history/plans/Plan-20260624` - Semver version + update-available
badge.md:
- Around line 1-56: This plan note should not live under docs/history because
that area is meant for backward-looking, pruned history only. Move the content
to the appropriate active planning/docs location outside docs/history, and leave
docs/history for archived lessons only; update any references in the plan
document if needed so the new location is easy to find.

In `@src/ui/app.js`:
- Around line 2167-2170: The firmware update check is spawning duplicate
concurrent GitHub requests because updateStatusBar() repeatedly calls
checkFirmwareUpdate(false) before the cache is refreshed. Add in-flight
de-duplication inside checkFirmwareUpdate() and/or getLatestStableRelease() so
that when a fetch is already pending, subsequent callers reuse the same promise
instead of starting another releases/latest request. Make sure the shared
promise is cleared after it settles, and keep the cache-first behavior intact
for the updateStatusBar() refresh path.

🪄 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: f3bf160c-9161-4a26-adcb-7553626cf46c

📥 Commits

Reviewing files that changed from the base of the PR and between 42f5ec1 and 84343f8.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (13)

• .github/workflows/release.yml
• docs/history/plans/Plan-20260624 - Semver version + update-available badge.md
• docs/moonmodules/core/FirmwareUpdateModule.md
• library.json
• src/core/FirmwareUpdateModule.h
• src/core/HttpServerModule.cpp
• src/ui/app.js
• src/ui/embed_ui.cmake
• src/ui/index.html
• src/ui/semver.js
• src/ui/style.css
• test/js/semver.test.mjs
• test/python/test_verify_version.py

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Move this plan out of docs/history.

This is active planning, but docs/history/**/*.md is meant to stay backward-looking and be pruned once the lesson is absorbed. Keeping PR plans here will make history notes grow indefinitely.

As per path instructions, docs/history/**/*.md: Keep history pages backward-looking and prune them when lessons have been absorbed; the permanent record is in git commits, not in indefinitely growing history notes.

🧰 Tools

🪛 LanguageTool

[style] ~6-~6: ‘On top of that’ might be wordy. Consider a shorter alternative.
Context: ...has a prerelease suffix → not stable." On top of that clean version, add a status-bar **"firm...

(EN_WORDINESS_PREMIUM_ON_TOP_OF_THAT)

---

[uncategorized] ~24-~24: Do not mix variants of the same word (‘pre-release’ and ‘prerelease’) within a single text.
Context: ...-dev/prerelease suffix marks a moving/pre-release build." ### 2. Reusable semver module ...

(EN_WORD_COHERENCY)

🪛 markdownlint-cli2 (0.22.1)

[warning] 9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 12-12: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 20-20: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 30-30: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 49-49: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

---

[warning] 52-52: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below

(MD022, blanks-around-headings)

🤖 Prompt for AI Agents

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

In `@docs/history/plans/Plan-20260624` - Semver version + update-available
badge.md around lines 1 - 56, This plan note should not live under docs/history
because that area is meant for backward-looking, pruned history only. Move the
content to the appropriate active planning/docs location outside docs/history,
and leave docs/history for archived lessons only; update any references in the
plan document if needed so the new location is easy to find.

Source: Path instructions

[coderabbitai]

Actionable comments posted: 9

Caution

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

⚠️ Outside diff range comments (1)

docs/history/plans/Plan-20260624 - Dev-channel update badge.md (1)

1-49: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Move this active plan out of docs/history/.

This page is a forward-looking implementation plan, but docs/history/** is reserved for backward-looking notes that get pruned once the lessons are absorbed. Keeping live planning docs here pushes the history tree against the documented convention.

As per coding guidelines, "Keep history pages backward-looking and prune them when lessons have been absorbed; the permanent record is in git commits, not in indefinitely growing history notes."

🤖 Prompt for AI Agents

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

In `@docs/history/plans/Plan-20260624` - Dev-channel update badge.md around lines
1 - 49, The plan document is forward-looking but lives under the history docs,
which should only contain backward-looking notes. Move this plan out of
docs/history/ and into the appropriate active planning/docs location, preserving
the content and using the same document title if needed; update any references
so the active plan is discoverable, and keep docs/history reserved for prunable
retrospective notes.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In @.github/workflows/release.yml:
- Around line 150-152: The release workflow currently maps every non-latest tag
to stable, so RC tags like v2.1.0-rc1 get the plain core version instead of an
RC-aware version. Update the version computation logic in the release job’s
compute-version step to branch explicitly for prerelease/RC tags the same way as
the other version step, and ensure it passes the correct channel into
scripts/build/compute_version.py or the uv run equivalent. Also add a test
alongside test/python/test_compute_version.py that asserts RC tags produce a
prerelease version rather than collapsing to stable X.Y.Z.

In `@docs/install/install.css`:
- Line 233: Replace the deprecated long-token wrapping rule in the stylesheet by
updating the .bd-val declaration to use overflow-wrap: anywhere instead of
word-break: break-word, and apply the same change in the other matching .bd-val
rule referenced by the review; keep the existing flex/min-width behavior intact
while adjusting the wrapping property.

In `@docs/install/install.js`:
- Around line 510-518: The success modal in install.js assigns clickable URLs in
the done-url and done-url-mdns elements before validating them, so a non-http(s)
result can still become an unsafe link. Update the success-link handling around
the code that sets a.href and aMdns.href to only assign href after the same
http(s)-only validation used by myDevices.addProvisionedDevice(), and skip or
neutralize rendering for invalid values.
- Around line 489-501: The success flow in install.js shows the “Device is
online!” header even when no device URL is available, so update the post-flash
handling around the no-URL branch to avoid setting that online state in the
first place. Adjust the logic in the success path and the no-url handling so
that when `url` is missing, the done section uses a neutral/offline message and
only shows the online header when a real device address exists; keep the
behavior localized to the `showSection("done")` / `done-url` / `done-defaults`
flow.
- Around line 876-883: The erase-only success handler is leaving the page in a
stale “done” state, so update the `onSuccess` flow to explicitly reset the
unload guard and clear any previous `done-status` content before showing the
erase completion message. Use the existing `onSuccess` callback in
`docs/install/install.js` and the `showSection("done")` / `done-url` /
`done-url-mdns` / `done-defaults` logic to locate the right spot, and make sure
the erase path does not inherit install-only status or tab-close warnings.

In `@src/ui/app.js`:
- Around line 2274-2275: The dev manifest cache key is shared across firmware
variants even though the URL in the manifest fetch is firmware-specific, so
update the cachedJson call in the dev update path to include dev.firmware in the
cache slot key. Use the existing manifest-loading logic around the url/manifest
assignment in app.js to make the cache entry unique per firmware so one variant
cannot reuse another variant’s latest manifest state.
- Around line 2264-2266: The release asset lookup in the update matching logic
only checks the tagged version form, so stable binaries named with the cleaned
semver are missed. Update the asset-name check in the matching flow around the
version comparison to also consider the pure version derived from rel.tag_name
(without the leading v) when evaluating hasBinary, using the existing release
selection logic and identifiers like isNewer, rel.tag_name, dev.version, and
assetNames.
- Around line 2225-2229: The stale-cache fallback in update should also refresh
the cache metadata so it does not keep retrying on every status tick. In the
catch block that serves data from safeLocalGet(key), update the stored
timestamp/expiry for that key before returning the parsed cached payload, using
the same cache entry logic already used by the successful fetch path in update
so the fallback remains valid until the next intended refresh.

In `@test/python/test_compute_version.py`:
- Around line 28-48: The version computation tests do not cover the no-tag
fallback path promised by the docstring. Add a test in test_compute_version.py
that exercises compute() or the underlying helper when no v* tag exists,
verifying it falls back to rev-list --count HEAD behavior for the first-release
case; use monkeypatch on the relevant helper(s) like commits_since_last_stable
and the version source via LIBRARY_JSON to keep the test deterministic.

---

Outside diff comments:
In `@docs/history/plans/Plan-20260624` - Dev-channel update badge.md:
- Around line 1-49: The plan document is forward-looking but lives under the
history docs, which should only contain backward-looking notes. Move this plan
out of docs/history/ and into the appropriate active planning/docs location,
preserving the content and using the same document title if needed; update any
references so the active plan is discoverable, and keep docs/history reserved
for prunable retrospective notes.

🪄 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: 4cd67847-7e82-47c2-a78f-d5f8921ecfb5

📥 Commits

Reviewing files that changed from the base of the PR and between 84343f8 and aec2e20.

⛔ Files ignored due to path filters (3)

• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/compute_version.py is excluded by !**/build/**
• scripts/build/generate_build_info.py is excluded by !**/build/**

📒 Files selected for processing (11)

• .github/workflows/release.yml
• docs/history/plans/Plan-20260624 - Dev-channel update badge.md
• docs/install/index.html
• docs/install/install.css
• docs/install/install.js
• docs/moonmodules/core/FirmwareUpdateModule.md
• esp32/main/CMakeLists.txt
• src/ui/app.js
• test/js/semver.test.mjs
• test/python/test_compute_version.py
• test/scenarios/light/scenario_Audio_mutation.json

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

.github/workflows/release.yml (2)

146-153: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Centralize the tag→version resolution.

These two jobs still duplicate the same release-tag/channel/version mapping. The next semver-rule edit can land in one copy and silently desync the binary MM_VERSION, asset filenames, and manifest --version. Move this logic behind one checked-in helper and call it from both places.

Based on learnings, "Do not duplicate logic or facts across code or documentation; shared behavior belongs in shared functions, and repeated facts in docs should be consolidated and linked instead of restated."

Also applies to: 304-311

🤖 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 @.github/workflows/release.yml around lines 146 - 153, The release-tag to
version mapping is duplicated in the workflow, which can desync version-related
outputs; centralize this logic in one checked-in helper and reuse it from the
Compute version step and the other matching job so binary MM_VERSION, asset
filenames, and manifest --version all come from the same source. Keep the shared
behavior behind a single helper script/function (for example the existing
compute_version.py path or a new wrapper) and update both workflow spots to call
it instead of re-implementing the channel/tag branching inline.

Source: Learnings

---

146-152: 📐 Maintainability & Code Quality | 🔵 Trivial

Replace raw python with uv run in the compute version step.

The "Compute version" step at line 152 invokes python directly, whereas line 310 in the same workflow correctly uses uv run python for the same script. Unify these execution paths to align with repository standards and ensure consistent runtime environments.

Suggested change

          set -euo pipefail
          if [ "${{ steps.tag.outputs.tag }}" = "latest" ]; then CH=latest; else CH=stable; fi
          # Pass the tag so an -rc tag yields its own prerelease semver (not the core).
-          V=$(python scripts/build/compute_version.py --channel "$CH" --tag "${{ steps.tag.outputs.tag }}")
+          V=$(uv run python scripts/build/compute_version.py --channel "$CH" --tag "${{ steps.tag.outputs.tag }}")

🤖 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 @.github/workflows/release.yml around lines 146 - 152, The Compute version
step still calls the compute_version.py script with raw python, which should be
aligned with the rest of the workflow. Update the run block in the ver step to
use uv run python for scripts/build/compute_version.py, matching the existing
execution pattern used elsewhere in this workflow and keeping the environment
consistent.

Source: Learnings

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/architecture.md`:
- Around line 354-356: Update the modifier wording in the architecture docs so
it does not imply chained modifier execution is implemented today: in the
section describing Layer and Layer::rebuildLUT, keep the contract that only the
first enabled modifier is applied, and either remove the sequencing examples or
clearly label them as future/backlog behavior. Use the existing symbols
Layer::rebuildLUT, modifiers, and backlog/README.md to keep the text aligned
with current behavior.

In `@docs/backlog/livescripts-analysis-top-down.md`:
- Around line 372-373: Update the Stage 0.5 acceptance text in the backlog doc
so it validates seam correctness by observable behavior rather than requiring
front-end and IR files to be byte-identical. Keep the existing references to the
same input, same front-end/IR path, and same rendered output, but replace the
file-identity requirement with a behavior-based proof that both backends (RISC-V
P4 and desktop x86-64) produce equivalent results.
- Line 427: The current wording conflates the tutorial hello-world spike with
the fix-warnings crash repro; revise the sentence in the
livescripts-analysis-top-down doc to keep them clearly separate. Update the text
around the Ripples comparison and the `setRGB(random16, blue)` reference so it
says the hello-world script is the tutorial spike, while the `fix-warnings` fork
is the distinct regression case. Use the existing terms `Ripples`,
`setRGB(random16, blue)`, and `fix-warnings` to anchor the distinction.
- Around line 216-219: The safety-tier section conflates runtime protections
with the parser crash from `fix-warnings`; update the discussion around the
nested-arg null-deref to separate AST/parsing validation from runtime bounds
checks and the watchdog. In the `livescripts-analysis-top-down` content, call
out that missing AST children must be handled by parser/analysis validation and
backed by a regression test, while keeping the runtime tier limited to `setRGB`,
indexed access, and watchdog-style protections.

---

Outside diff comments:
In @.github/workflows/release.yml:
- Around line 146-153: The release-tag to version mapping is duplicated in the
workflow, which can desync version-related outputs; centralize this logic in one
checked-in helper and reuse it from the Compute version step and the other
matching job so binary MM_VERSION, asset filenames, and manifest --version all
come from the same source. Keep the shared behavior behind a single helper
script/function (for example the existing compute_version.py path or a new
wrapper) and update both workflow spots to call it instead of re-implementing
the channel/tag branching inline.
- Around line 146-152: The Compute version step still calls the
compute_version.py script with raw python, which should be aligned with the rest
of the workflow. Update the run block in the ver step to use uv run python for
scripts/build/compute_version.py, matching the existing execution pattern used
elsewhere in this workflow and keeping the environment consistent.

🪄 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: 08dea03e-dd73-42d9-af57-84122ab14a98

📥 Commits

Reviewing files that changed from the base of the PR and between aec2e20 and 12e2ddc.

⛔ Files ignored due to path filters (3)

• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/compute_version.py is excluded by !**/build/**
• scripts/build/setup_esp_idf.py is excluded by !**/build/**

📒 Files selected for processing (26)

• .github/workflows/release.yml
• CLAUDE.md
• docs/architecture.md
• docs/backlog/README.md
• docs/backlog/backlog-core.md
• docs/backlog/backlog-light.md
• docs/backlog/backlog-mixed.md
• docs/backlog/leddriver-deferred.md
• docs/backlog/livescripts-analysis-bottom-up.md
• docs/backlog/livescripts-analysis-top-down.md
• docs/backlog/ui-deferred.md
• docs/building.md
• docs/history/README.md
• docs/history/v1-inventory.md
• docs/install/install.css
• docs/install/install.js
• docs/moonmodules/core/AudioModule.md
• docs/moonmodules/light/BlendMap.md
• docs/moonmodules/light/Drivers.md
• docs/moonmodules/light/Layers.md
• docs/moonmodules/light/drivers/LcdLedDriver.md
• docs/moonmodules/light/drivers/NetworkSendDriver.md
• docs/performance.md
• src/ui/app.js
• test/python/test_compute_version.py
• test/scenarios/light/scenario_Audio_mutation.json

💤 Files with no reviewable changes (2)

• docs/backlog/leddriver-deferred.md
• docs/backlog/ui-deferred.md

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clarify that modifier chaining is still backlog-only.

The new example reads like multiply-then-checkerboard vs checkerboard-then-multiply already changes behavior, but the current contract still applies only the first enabled modifier during Layer::rebuildLUT. Please remove the sequencing example or mark it explicitly as future behavior.

♻️ Suggested rewording

-A layer applies its **first enabled modifier** during LUT build (`Layer::rebuildLUT`). Modifiers are **reorderable** in the UI, and order is meaningful (a multiply-then-checkerboard mask differs from checkerboard-then-multiply, just as mirror-then-rotate differs from rotate-then-mirror). Applying several modifiers in sequence (chaining) is on the [backlog](backlog/README.md).
+A layer applies its **first enabled modifier** during LUT build (`Layer::rebuildLUT`). Modifiers are **reorderable** in the UI so users can choose which one is active first. Applying several modifiers in sequence (chaining) is on the [backlog](backlog/README.md).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	A layer can have **multiple effects**. Each effect writes to the buffer sequentially in its listed order, overwriting or adding to the previous — so the effects stack (a base-colour effect followed by a sparkle effect).
	A layer applies its **first enabled modifier** during LUT build (`Layer::rebuildLUT`). Modifier *chaining* (applying several in sequence) is not implemented: only the first enabled modifier takes effect. Order matters for a chain (a multiply-then-checkerboard mask differs from checkerboard-then-multiply, just as mirror-then-rotate differs from rotate-then-mirror), which is why modifiers are reorderable in the UI even though only the first is applied today. Chaining is on the [backlog](backlog/backlog.md): static modifiers chain during LUT build, dynamic modifiers during rendering.
	A layer applies its **first enabled modifier** during LUT build (`Layer::rebuildLUT`). Modifiers are **reorderable** in the UI, and order is meaningful (a multiply-then-checkerboard mask differs from checkerboard-then-multiply, just as mirror-then-rotate differs from rotate-then-mirror). Applying several modifiers in sequence (chaining) is on the [backlog](backlog/README.md).
	A layer can have **multiple effects**. Each effect writes to the buffer sequentially in its listed order, overwriting or adding to the previous — so the effects stack (a base-colour effect followed by a sparkle effect).
	A layer applies its **first enabled modifier** during LUT build (`Layer::rebuildLUT`). Modifiers are **reorderable** in the UI so users can choose which one is active first. Applying several modifiers in sequence (chaining) is on the [backlog](backlog/README.md).

🤖 Prompt for AI Agents

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

In `@docs/architecture.md` around lines 354 - 356, Update the modifier wording in
the architecture docs so it does not imply chained modifier execution is
implemented today: in the section describing Layer and Layer::rebuildLUT, keep
the contract that only the first enabled modifier is applied, and either remove
the sequencing examples or clearly label them as future/backlog behavior. Use
the existing symbols Layer::rebuildLUT, modifiers, and backlog/README.md to keep
the text aligned with current behavior.

[coderabbitai]

Note

Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it.

---

Generating unit tests... This may take up to 20 minutes.

[coderabbitai]

✅ Created PR with unit tests: #28

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

src/ui/app.js (1)

2259-2284: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Use a CORS-readable source for dev-channel update checks.

This path now treats the device-hosted fetch as an expected CORS failure and negative-caches null, so a -dev device cannot reliably discover newer latest builds from the embedded UI. Move the dev check to a CORS-readable GitHub API endpoint or another CORS-enabled manifest source instead of relying on release-asset downloads.

🤖 Prompt for AI Agents

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

In `@src/ui/app.js` around lines 2259 - 2284, The update-check path in
src/ui/app.js is using a release-asset fetch that is expected to fail with CORS
from the device-hosted UI and then negative-caches null, which prevents reliable
discovery of newer -dev builds. Update the dev-channel logic around the
fetch/update check to use a CORS-readable source such as a GitHub API endpoint
or another CORS-enabled manifest, and keep the fallback/TTL behavior in the same
update-check code path so it only serves stale data when the CORS-friendly
source is unavailable.

test/scenarios/core/scenario_MoonModule_control_change.json (1)

121-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

The refreshed macOS sample now exceeds this step's own contract.

This baseline step records pc-macos.tick_us = 316 on June 25, 2026, but the nearby contract still caps the same metric at 120. Either fix the regression or update the contract in the same change; otherwise the scenario's recorded telemetry and acceptance gate disagree.

🤖 Prompt for AI Agents

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

In `@test/scenarios/core/scenario_MoonModule_control_change.json` around lines 121
- 133, The baseline step in scenario_MoonModule_control_change now records a
pc-macos.tick_us value that exceeds the step’s contract, so align the telemetry
and acceptance gate in the same update. Either bring the recorded value back
within the existing limit or update the contract threshold where the baseline
step is defined, keeping the metric and its cap consistent for the MoonModule
control change scenario.

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

93-105: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

This refreshed sample also breaks the declared macOS performance gate.

The step now records pc-macos.tick_us = 272 on June 25, 2026, while the contract for the same step still requires 120. Please either keep the old bound by fixing the slowdown or revise the contract alongside this telemetry refresh.

🤖 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_MultiplyModifier_pipeline.json` around lines 93
- 105, The refreshed telemetry sample in the MultiplyModifier pipeline now
exceeds the declared macOS performance gate, so the sample or contract is out of
sync. Update the scenario data in scenario_MultiplyModifier_pipeline so the
step’s macOS tick_us value is brought back within the existing 120 bound, or if
the new value is intentional, revise the corresponding contract entry for that
step to match the new telemetry; use the step’s pc-macos.tick_us and the
associated contract metadata to locate the mismatch.

🤖 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 @.github/workflows/release.yml:
- Around line 153-155: The version computation steps are interpolating the tag
directly inside the run script, which can allow shell injection; update the
release workflow so the tag from steps.tag.outputs.tag is passed via env and
then referenced inside the script with normal shell expansion. Apply the same
fix in both compute_version invocations in the workflow, keeping the logic in
the step but removing direct GitHub Actions interpolation from the command text.

In `@docs/tests/unit-tests.md`:
- Line 208: Rewrite the bullet in present tense only, removing the past-tense
phrase “Moved here from SystemModule” and keeping the current ownership
description for the firmware card; update the wording in the documentation entry
so it describes the `firmware` control and partition usage as current behavior
without referencing the migration history.

In `@src/light/drivers/Drivers.h`:
- Around line 175-176: The driver wiring still treats a non-null layer_ as valid
even when Layers::enabledLayerCount() is zero, so activeLayer() can be passed
through as stale output instead of going idle. Update the logic in Drivers.h
around needOutput, the single-layer LUT path, and buf selection to require at
least one enabled layer before using container-backed layer_ data. Use
layers_->enabledLayerCount(), layer_->lut().hasLUT(), and activeLayer()
consistently so the fallback only applies when there is an enabled source layer.

In `@src/light/layers/Layers.h`:
- Around line 64-67: `enabledLayerCount()` is now exposing the no-enabled case,
but `activeLayer()` still falls back to a disabled registered layer, so driver
output can keep using stale single-layer data. Add an enabled-only accessor such
as `firstEnabledLayer()` in `Layers.h` and use it in `Drivers::loop()` and any
other output-selection paths that must respect enablement; keep `activeLayer()`
only for geometry/fallback callers if that behavior is still required.

---

Outside diff comments:
In `@src/ui/app.js`:
- Around line 2259-2284: The update-check path in src/ui/app.js is using a
release-asset fetch that is expected to fail with CORS from the device-hosted UI
and then negative-caches null, which prevents reliable discovery of newer -dev
builds. Update the dev-channel logic around the fetch/update check to use a
CORS-readable source such as a GitHub API endpoint or another CORS-enabled
manifest, and keep the fallback/TTL behavior in the same update-check code path
so it only serves stale data when the CORS-friendly source is unavailable.

In `@test/scenarios/core/scenario_MoonModule_control_change.json`:
- Around line 121-133: The baseline step in scenario_MoonModule_control_change
now records a pc-macos.tick_us value that exceeds the step’s contract, so align
the telemetry and acceptance gate in the same update. Either bring the recorded
value back within the existing limit or update the contract threshold where the
baseline step is defined, keeping the metric and its cap consistent for the
MoonModule control change scenario.

In `@test/scenarios/light/scenario_MultiplyModifier_pipeline.json`:
- Around line 93-105: The refreshed telemetry sample in the MultiplyModifier
pipeline now exceeds the declared macOS performance gate, so the sample or
contract is out of sync. Update the scenario data in
scenario_MultiplyModifier_pipeline so the step’s macOS tick_us value is brought
back within the existing 120 bound, or if the new value is intentional, revise
the corresponding contract entry for that step to match the new telemetry; use
the step’s pc-macos.tick_us and the associated contract metadata to locate the
mismatch.

🪄 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: f67e2e1d-2f73-4271-9002-415d882bc5f2

📥 Commits

Reviewing files that changed from the base of the PR and between 12e2ddc and 6e61885.

⛔ Files ignored due to path filters (1)

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

📒 Files selected for processing (29)

• .github/workflows/release.yml
• docs/architecture.md
• docs/backlog/README.md
• docs/backlog/backlog-core.md
• docs/backlog/livescripts-analysis-top-down.md
• docs/history/plans/Plan-20260625 - Multi-layer composition.md
• docs/moonmodules/light/BlendMap.md
• docs/moonmodules/light/Drivers.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/Layers.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• src/light/drivers/Drivers.h
• src/light/layers/BlendMap.h
• src/light/layers/Layer.h
• src/light/layers/Layers.h
• src/ui/app.js
• test/python/test_compute_version.py
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/light/scenario_Audio_mutation.json
• test/scenarios/light/scenario_Driver_mutation.json
• test/scenarios/light/scenario_Layers_composition.json
• test/scenarios/light/scenario_Layouts_mutation.json
• test/scenarios/light/scenario_MultiplyModifier_pipeline.json
• test/scenarios/light/scenario_modifier_swap.json
• test/scenarios/light/scenario_perf_full.json
• test/scenarios/light/scenario_perf_light.json
• test/unit/light/unit_BlendMap.cpp
• test/unit/light/unit_Layers_container.cpp

[coderabbitai]

Actionable comments posted: 7

Caution

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

⚠️ Outside diff range comments (2)

docs/backlog/backlog-core.md (2)

91-100: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Tighten the doc-asset exposure before calling this “not blocking.”

This endpoint can already serve arbitrary repo-local files to any LAN client, including credential artefacts if present. That is a security/privacy risk, not just a hardening nicety.

🤖 Prompt for AI Agents

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

In `@docs/backlog/backlog-core.md` around lines 91 - 100, The MoonDeck doc-asset
endpoint is too permissive and can serve sensitive repo-local files to LAN
clients. Update scripts/moondeck.py::_serve_doc_asset to restrict serving to an
explicit subdirectory allowlist (for example docs and the image assets needed by
the renderer) and add a small extension/MIME allowlist so only intended static
assets are returned. Keep the existing path traversal protection, but reject any
non-allowed asset paths with 403, and consider adding the optional --bind
127.0.0.1 flag in main() if you expose that control here.

---

183-189: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Keep board presets live-applied.

This plan persists the preset and applies it on the next boot, which conflicts with the repo’s no-reboot config rule and makes board selection behave differently from every other control. Split the static catalog from the live-applied controls, or call out an explicit exception if the hardware boundary really blocks hot reconfiguration.

As per coding guidelines, “No reboot to apply a configuration change. Every setting takes effect live, on the next render tick...”.

🤖 Prompt for AI Agents

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

In `@docs/backlog/backlog-core.md` around lines 183 - 189, The board preset flow
currently defers applying changes until reboot, which conflicts with the
live-apply rule used elsewhere. Update the board preset design so the static
catalog remains separate from the live-applied control path, and make the
`/api/board-preset` handling plus MoonDeck “Set board” selection apply changes
immediately where possible via the relevant runtime/bootstrap logic instead of
waiting for the next boot. If any hardware-only setting in the preset truly
cannot be hot-swapped, explicitly isolate that exception in the board preset
workflow and document it in the same place that defines the live application
behavior.

Source: Coding guidelines

♻️ Duplicate comments (1)

docs/tests/unit-tests.md (1)

194-194: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Rewrite this regression bullet in present tense.

were clamped and became break the docs tense rule. Keep the entry as a current contract instead of a changelog note.

Suggested edit

-- Regression: Int16 controls round-tripped through the filesystem load path were clamped to c.min/c.max, which default to 0,0 because ControlDescriptor.min/max are uint8_t and can't represent an int16 range. Every Int16 control loaded as 0 — so a 128×128 grid became 0×0×0 after restart and the whole pipeline allocated no buffers. Int16 controls now preserve their saved value across load — no zero-clamping from uint8 min/max bounds.
+- Regression: Int16 controls preserve their saved value across filesystem load; the load path does not zero-clamp them through `uint8_t` min/max bounds in `ControlDescriptor`.

As per coding guidelines, **/*.{md,cpp,h,js,ts,py,cmake,json} must "Write code, comments, and documentation in the present tense only; avoid future-tense, changelog-style, and absence-narration phrasing outside docs/backlog/ and docs/history/ and the stated 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 `@docs/tests/unit-tests.md` at line 194, Rewrite the regression note in present
tense so it reads as a current behavior contract rather than a past event;
update the bullet text under the Int16 controls regression entry to avoid
phrases like “were clamped” and “became,” and instead describe the filesystem
load path behavior using the same symbols and context around
ControlDescriptor.min/max, Int16 controls, and the saved value preservation.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@docs/history/plans/Plan-20260625` - RegionModifier (start-end carving).md:
- Around line 23-32: Update the rounding rule in the plan document so it matches
the half-open interval behavior used by RegionModifier: align the spec with
src/light/modifiers/RegionModifier.h and
docs/moonmodules/light/modifiers/RegionModifier.md by replacing the inclusive
endPixel and width formula with the half-open [startPixel, endPixel)
interpretation. Make sure the language around clamping and the example under the
rounding rule are consistent with the implementation’s start/end math.

In `@docs/moonmodules/core/FirmwareUpdateModule.md`:
- Line 17: The inline code span in the OTA flash phase description has invalid
spacing around the error prefix, causing the markdown lint issue. Update the
wording in FirmwareUpdateModule.md so the `error:` prefix is written without any
trailing space inside the backticks, while keeping the rest of the phase list
and the `MoonModule::setStatus()` / `Severity::Error` mapping text unchanged.

In `@docs/moonmodules/light/modifiers/RegionModifier.md`:
- Line 5: Clarify the RegionModifier docs so the current behavior is stated
first and chaining is clearly marked as future work. In the Region/Multiply
description, update the wording in RegionModifier.md to say that a Layer
currently applies only its first enabled modifier, and mention modifier chaining
(Region then Multiply) only as planned/backlog context, using the Region and
Multiply symbols to keep the explanation anchored to the existing modifiers.

In `@src/light/drivers/Drivers.h`:
- Around line 256-264: The layer propagation in the driver loop is using the
wrong source for geometry fallback: keep `out` for selecting the buffer/LUT, but
change the `DriverBase::setLayer()` call in `Drivers::` the child iteration to
pass the already-resolved `layer_` instead of `out`. This preserves
`activeLayer()` geometry when all layers are disabled while still using
`firstEnabledLayer()` only for `buf` selection, and keeps preview-style drivers
from losing width/height/depth.

In `@src/ui/app.js`:
- Around line 2275-2279: The failure-cache comment in the status-bar fetch logic
uses past tense (“re-ran”), which should be rewritten in present tense to match
the repo’s comment style. Update the descriptive comment near the negative-cache
handling in the app render/fetch path so it describes the behavior as current
and ongoing, using the existing context around the stale-entry suppression and
failing fetch retry flow.

In `@test/unit/light/unit_Layer_sparse_mapping.cpp`:
- Around line 215-227: The sparse mapping test currently only checks the total
destination count and that all destinations stay inside the carved region, which
still allows duplicate physical lights and a missing target. Update the
validation in the loop over layer.lut().logicalCount() and forEachDestination to
track each destination index by its unique physical light identifier, then
assert that all 16 destinations are distinct in addition to remaining within the
4x4 region. Keep the checks centered on the 1:1 carved mapping behavior so the
test fails if any logical cell collapses onto the same destination or any cell
is never reached.

In `@test/unit/light/unit_Layers_container.cpp`:
- Around line 299-326: The stale-buffer check in the driver test is too weak
because it only covers the already-disabled initial state. Update the scenario
around Layers, Drivers, and CaptureDriver so it first publishes a frame with the
layer enabled, then disables the layer and rebuilds, and only then asserts the
source buffer is cleared. Keep the existing assertions on activeLayer(),
firstEnabledLayer(), enabledLayerCount(), dynamicBytes(), and cap.src_ to verify
the transition from a valid frame to nullptr.

---

Outside diff comments:
In `@docs/backlog/backlog-core.md`:
- Around line 91-100: The MoonDeck doc-asset endpoint is too permissive and can
serve sensitive repo-local files to LAN clients. Update
scripts/moondeck.py::_serve_doc_asset to restrict serving to an explicit
subdirectory allowlist (for example docs and the image assets needed by the
renderer) and add a small extension/MIME allowlist so only intended static
assets are returned. Keep the existing path traversal protection, but reject any
non-allowed asset paths with 403, and consider adding the optional --bind
127.0.0.1 flag in main() if you expose that control here.
- Around line 183-189: The board preset flow currently defers applying changes
until reboot, which conflicts with the live-apply rule used elsewhere. Update
the board preset design so the static catalog remains separate from the
live-applied control path, and make the `/api/board-preset` handling plus
MoonDeck “Set board” selection apply changes immediately where possible via the
relevant runtime/bootstrap logic instead of waiting for the next boot. If any
hardware-only setting in the preset truly cannot be hot-swapped, explicitly
isolate that exception in the board preset workflow and document it in the same
place that defines the live application behavior.

---

Duplicate comments:
In `@docs/tests/unit-tests.md`:
- Line 194: Rewrite the regression note in present tense so it reads as a
current behavior contract rather than a past event; update the bullet text under
the Int16 controls regression entry to avoid phrases like “were clamped” and
“became,” and instead describe the filesystem load path behavior using the same
symbols and context around ControlDescriptor.min/max, Int16 controls, and the
saved value preservation.

🪄 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: 2aced0a2-959c-43e0-80ed-ba4fcae9f463

📥 Commits

Reviewing files that changed from the base of the PR and between 6e61885 and 476535d.

📒 Files selected for processing (28)

• .github/workflows/release.yml
• CLAUDE.md
• docs/architecture.md
• docs/backlog/README.md
• docs/backlog/backlog-core.md
• docs/backlog/backlog-light.md
• docs/backlog/backlog-mixed.md
• docs/history/decisions.md
• docs/history/plans/Plan-20260625 - RegionModifier (start-end carving).md
• docs/moonmodules/core/Control.md
• docs/moonmodules/core/FirmwareUpdateModule.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/modifiers/RegionModifier.md
• docs/tests/unit-tests.md
• src/core/Control.h
• src/core/FirmwareUpdateModule.h
• src/light/drivers/Drivers.h
• src/light/layers/Layer.h
• src/light/layers/Layers.h
• src/light/modifiers/RegionModifier.h
• src/main.cpp
• src/ui/app.js
• test/CMakeLists.txt
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_FirmwareUpdateModule.cpp
• test/unit/light/unit_Layer_sparse_mapping.cpp
• test/unit/light/unit_Layers_container.cpp
• test/unit/light/unit_RegionModifier.cpp

💤 Files with no reviewable changes (1)

• src/light/layers/Layer.h

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The rounding rule here no longer matches the implementation.

This section still specifies an inclusive endPixel and end - start + 1, but src/light/modifiers/RegionModifier.h and docs/moonmodules/light/modifiers/RegionModifier.md in this PR implement half-open [startPixel, endPixel) math. Leaving both versions in-tree makes the approved plan disagree with the shipped code.

🤖 Prompt for AI Agents

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

In `@docs/history/plans/Plan-20260625` - RegionModifier (start-end carving).md
around lines 23 - 32, Update the rounding rule in the plan document so it
matches the half-open interval behavior used by RegionModifier: align the spec
with src/light/modifiers/RegionModifier.h and
docs/moonmodules/light/modifiers/RegionModifier.md by replacing the inclusive
endPixel and width formula with the half-open [startPixel, endPixel)
interpretation. Make sure the language around clamping and the example under the
rounding rule are consistent with the implementation’s start/end math.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the code-span spacing on Line 17.

error: inside the inline code span trips MD038. Spell the prefix without spaces inside the backticks so the docs stay lint-clean.

Suggested fix

-An `error: ` prefix maps to `Severity::Error`; `idle` clears the banner; everything else is neutral `Severity::Status`.
+An `error:` prefix maps to `Severity::Error`; `idle` clears the banner; everything else is neutral `Severity::Status`.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

The OTA flash phase (`idle`, `starting`, `downloading`, `flashing`, `rebooting`, `error: <reason>`) is not a control — it surfaces through the module's shared status slot (`MoonModule::setStatus()`), the same per-module banner every module uses (NetworkModule's IP line, DevicesModule's sweep count). An `error: ` prefix maps to `Severity::Error`; `idle` clears the banner; everything else is neutral `Severity::Status`.

The OTA flash phase (`idle`, `starting`, `downloading`, `flashing`, `rebooting`, `error: <reason>`) is not a control — it surfaces through the module's shared status slot (`MoonModule::setStatus()`), the same per-module banner every module uses (NetworkModule's IP line, DevicesModule's sweep count). An `error:` prefix maps to `Severity::Error`; `idle` clears the banner; everything else is neutral `Severity::Status`.

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 17-17: Spaces inside code span elements

(MD038, no-space-in-code)

🤖 Prompt for AI Agents

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

In `@docs/moonmodules/core/FirmwareUpdateModule.md` at line 17, The inline code
span in the OTA flash phase description has invalid spacing around the error
prefix, causing the markdown lint issue. Update the wording in
FirmwareUpdateModule.md so the `error:` prefix is written without any trailing
space inside the backticks, while keeping the rest of the phase list and the
`MoonModule::setStatus()` / `Severity::Error` mapping text unchanged.

Source: Linters/SAST tools

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Describe chaining as planned, not current behavior.

The first sentence says Region and Multiply are composable, but the second says only the first enabled modifier applies today. That reads as if stacking already works. Please state the current contract first and leave composition as backlog/future context.

🤖 Prompt for AI Agents

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

In `@docs/moonmodules/light/modifiers/RegionModifier.md` at line 5, Clarify the
RegionModifier docs so the current behavior is stated first and chaining is
clearly marked as future work. In the Region/Multiply description, update the
wording in RegionModifier.md to say that a Layer currently applies only its
first enabled modifier, and mention modifier chaining (Region then Multiply)
only as planned/backlog context, using the Region and Multiply symbols to keep
the explanation anchored to the existing modifiers.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep DriverBase::layer_ on the geometry fallback.

out is the right source for buffer/LUT selection, but using it for drv->setLayer() throws away the activeLayer() fallback when every layer is disabled. That leaves preview-style drivers with no width/height/depth even though the physical topology has not changed. Pass the already-resolved layer_ into setLayer(), and keep firstEnabledLayer() only for buf selection.

Minimal fix

-        Layer* const out = layers_ ? layers_->firstEnabledLayer() : layer_;
+        Layer* const geom = layer_;  // activeLayer() fallback or pinned layer
+        Layer* const out = layers_ ? layers_->firstEnabledLayer() : layer_;
         const bool composing = layers_ && layers_->enabledLayerCount() > 1;
         Buffer* buf = out ? ((composing || out->lut().hasLUT()) ? &outputBuffer_
                                                                : &out->buffer())
                           : nullptr;
         for (uint8_t i = 0; i < childCount(); i++) {
             auto* drv = static_cast<DriverBase*>(child(i));
             drv->setSourceBuffer(buf);
-            drv->setLayer(out);  // null when no enabled Layer; drivers must tolerate it
+            drv->setLayer(geom);
             drv->setCorrection(&correction_);  // physical drivers apply it; Preview ignores
         }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	Layer* const out = layers_ ? layers_->firstEnabledLayer() : layer_;
	const bool composing = layers_ && layers_->enabledLayerCount() > 1;
	Buffer* buf = out ? ((composing || out->lut().hasLUT()) ? &outputBuffer_
	: &out->buffer())
	: nullptr;
	for (uint8_t i = 0; i < childCount(); i++) {
	auto* drv = static_cast<DriverBase*>(child(i));
	drv->setSourceBuffer(buf);
	drv->setLayer(layer_); // null when no active Layer; drivers must tolerate it
	drv->setLayer(out); // null when no enabled Layer; drivers must tolerate it
	Layer* const geom = layer_; // activeLayer() fallback or pinned layer
	Layer* const out = layers_ ? layers_->firstEnabledLayer() : layer_;
	const bool composing = layers_ && layers_->enabledLayerCount() > 1;
	Buffer* buf = out ? ((composing || out->lut().hasLUT()) ? &outputBuffer_
	: &out->buffer())
	: nullptr;
	for (uint8_t i = 0; i < childCount(); i++) {
	auto* drv = static_cast<DriverBase*>(child(i));
	drv->setSourceBuffer(buf);
	drv->setLayer(geom);
	drv->setCorrection(&correction_); // physical drivers apply it; Preview ignores

🤖 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 256 - 264, The layer propagation in
the driver loop is using the wrong source for geometry fallback: keep `out` for
selecting the buffer/LUT, but change the `DriverBase::setLayer()` call in
`Drivers::` the child iteration to pass the already-resolved `layer_` instead of
`out`. This preserves `activeLayer()` geometry when all layers are disabled
while still using `firstEnabledLayer()` only for `buf` selection, and keeps
preview-style drivers from losing width/height/depth.

[coderabbitai]

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the failure-cache comment in present tense.

Line 2278 uses past tense (“re-ran”), which conflicts with the repo-wide present-tense comment rule. Rephrase it as current behavior, e.g. “Without this, every status-bar render ... retries the failing fetch”.

🤖 Prompt for AI Agents

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

In `@src/ui/app.js` around lines 2275 - 2279, The failure-cache comment in the
status-bar fetch logic uses past tense (“re-ran”), which should be rewritten in
present tense to match the repo’s comment style. Update the descriptive comment
near the negative-cache handling in the app render/fetch path so it describes
the behavior as current and ongoing, using the existing context around the
stale-entry suppression and failing fetch retry flow.

Source: Coding guidelines

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert destination uniqueness, not just count and bounds.

total == 16 and insideRegion still pass if two logical cells collapse onto the same physical light while another cell is never targeted. Since this case is meant to pin a 1:1 carved mapping, track seen destinations and assert 16 unique indices.

Suggested addition

-    std::size_t total = 0;
-    bool insideRegion = true;
+    std::size_t total = 0;
+    bool insideRegion = true;
+    bool seen[64] = {};
+    std::size_t unique = 0;
     for (mm::nrOfLightsType li = 0; li < layer.lut().logicalCount(); li++) {
         layer.lut().forEachDestination(li, [&](mm::nrOfLightsType d) {
             total++;
             const mm::nrOfLightsType x = d % 8, y = d / 8;  // 8-wide box
             if (x >= 4 || y >= 4) insideRegion = false;
+            if (!seen[d]) {
+                seen[d] = true;
+                unique++;
+            }
         });
     }
     CHECK(total == 16);          // 4×4 region, 1:1, nothing outside
+    CHECK(unique == 16);         // no duplicate physical destinations
     CHECK(insideRegion);

As per path instructions, test/** should "Verify tests cover edge cases and match the specifications in docs/moonmodules/."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// Every destination is a real box light inside the carved quarter (x<4, y<4),
	// and there are exactly 16 of them (one per logical cell, no fan-out).
	std::size_t total = 0;
	bool insideRegion = true;
	for (mm::nrOfLightsType li = 0; li < layer.lut().logicalCount(); li++) {
	layer.lut().forEachDestination(li, [&](mm::nrOfLightsType d) {
	total++;
	const mm::nrOfLightsType x = d % 8, y = d / 8; // 8-wide box
	if (x >= 4 || y >= 4) insideRegion = false;
	});
	}
	CHECK(total == 16); // 4×4 region, 1:1, nothing outside
	CHECK(insideRegion);
	// Every destination is a real box light inside the carved quarter (x<4, y<4),
	// and there are exactly 16 of them (one per logical cell, no fan-out).
	std::size_t total = 0;
	bool insideRegion = true;
	bool seen[64] = {};
	std::size_t unique = 0;
	for (mm::nrOfLightsType li = 0; li < layer.lut().logicalCount(); li++) {
	layer.lut().forEachDestination(li, [&](mm::nrOfLightsType d) {
	total++;
	const mm::nrOfLightsType x = d % 8, y = d / 8; // 8-wide box
	if (x >= 4 || y >= 4) insideRegion = false;
	if (!seen[d]) {
	seen[d] = true;
	unique++;
	}
	});
	}
	CHECK(total == 16); // 4×4 region, 1:1, nothing outside
	CHECK(unique == 16); // no duplicate physical destinations
	CHECK(insideRegion);

🧰 Tools

🪛 Clang (14.0.6)

[note] 219-219: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 220-220: nesting level increased to 2

(clang)

---

[note] 223-223: +3, including nesting penalty of 2, nesting level increased to 3

(clang)

---

[note] 223-223: +1

(clang)

---

[note] 226-226: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 226-226: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 226-226: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 227-227: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 227-227: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 227-227: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[warning] 220-220: parameter name 'd' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 222-222: multiple declarations in a single statement reduces readability

(readability-isolate-declaration)

---

[warning] 222-222: variable name 'x' is too short, expected at least 3 characters

(readability-identifier-length)

---

[warning] 222-222: variable name 'y' is too short, expected at least 3 characters

(readability-identifier-length)

---

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

(readability-braces-around-statements)

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_Layer_sparse_mapping.cpp` around lines 215 - 227, The
sparse mapping test currently only checks the total destination count and that
all destinations stay inside the carved region, which still allows duplicate
physical lights and a missing target. Update the validation in the loop over
layer.lut().logicalCount() and forEachDestination to track each destination
index by its unique physical light identifier, then assert that all 16
destinations are distinct in addition to remaining within the 4x4 region. Keep
the checks centered on the 1:1 carved mapping behavior so the test fails if any
logical cell collapses onto the same destination or any cell is never reached.

Source: Path instructions

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Exercise the disable-after-render transition before asserting stale-buffer clearing.

This case starts with the only layer already disabled, so cap.src_ == nullptr still passes if Drivers never clears a previously-published source buffer. Prime the driver with an enabled frame first, then disable/rebuild and assert the handoff resets to nullptr.

Suggested test shape

     {
         mm::Layouts layouts; mm::GridLayout grid;
         grid.width = 8; grid.height = 8; grid.depth = 1;
         layouts.addChild(&grid);
         mm::Layers layers;
         mm::Layer only; only.setChannelsPerLight(3);
         mm::CheckerboardEffect eff; only.addChild(&eff);
         mm::MultiplyModifier mirror; mirror.mirrorX = true; only.addChild(&mirror);
         layers.addChild(&only);
         layers.setLayouts(&layouts);
-        only.setEnabled(false);
         mm::Drivers drivers; CaptureDriver cap; drivers.addChild(&cap);
         drivers.setLayers(&layers);
+
+        layers.onBuildState(); drivers.onBuildState();
+        REQUIRE(cap.src_ != nullptr);   // seed a previously-published source
+
+        only.setEnabled(false);
         layers.onBuildState(); drivers.onBuildState();
 
         CHECK(layers.activeLayer() == &only);
         CHECK(layers.firstEnabledLayer() == nullptr);
         CHECK(layers.enabledLayerCount() == 0);
         CHECK(drivers.dynamicBytes() == 0);
         CHECK(cap.src_ == nullptr);
     }

Based on learnings, "Keep the system robust to any valid UI action or API-call sequence; crashes or hangs are bugs and must be pinned with tests."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	// --- Case 4: the only layer is DISABLED → drivers idle, no stale buffer ---
	// activeLayer() still surfaces the disabled layer (geometry stays queryable),
	// but output selection must use the *enabled* source — with none, the driver's
	// source buffer goes null so it emits nothing instead of re-sending the last
	// frame off the disabled layer.
	{
	mm::Layouts layouts; mm::GridLayout grid;
	grid.width = 8; grid.height = 8; grid.depth = 1;
	layouts.addChild(&grid);
	mm::Layers layers;
	mm::Layer only; only.setChannelsPerLight(3);
	mm::CheckerboardEffect eff; only.addChild(&eff);
	// A LUT modifier so the pre-fix bug would route through the output path —
	// proves the disabled gate, not just the no-LUT zero-copy branch.
	mm::MultiplyModifier mirror; mirror.mirrorX = true; only.addChild(&mirror);
	layers.addChild(&only);
	layers.setLayouts(&layouts);
	only.setEnabled(false);
	mm::Drivers drivers; CaptureDriver cap; drivers.addChild(&cap);
	drivers.setLayers(&layers);
	layers.onBuildState(); drivers.onBuildState();
	CHECK(layers.activeLayer() == &only); // fallback for geometry
	CHECK(layers.firstEnabledLayer() == nullptr);// no enabled source
	CHECK(layers.enabledLayerCount() == 0);
	CHECK(drivers.dynamicBytes() == 0); // no output buffer allocated
	CHECK(cap.src_ == nullptr); // driver idle — no stale frame
	// --- Case 4: the only layer is DISABLED → drivers idle, no stale buffer ---
	// activeLayer() still surfaces the disabled layer (geometry stays queryable),
	// but output selection must use the *enabled* source — with none, the driver's
	// source buffer goes null so it emits nothing instead of re-sending the last
	// frame off the disabled layer.
	{
	mm::Layouts layouts; mm::GridLayout grid;
	grid.width = 8; grid.height = 8; grid.depth = 1;
	layouts.addChild(&grid);
	mm::Layers layers;
	mm::Layer only; only.setChannelsPerLight(3);
	mm::CheckerboardEffect eff; only.addChild(&eff);
	// A LUT modifier so the pre-fix bug would route through the output path —
	// proves the disabled gate, not just the no-LUT zero-copy branch.
	mm::MultiplyModifier mirror; mirror.mirrorX = true; only.addChild(&mirror);
	layers.addChild(&only);
	layers.setLayouts(&layouts);
	mm::Drivers drivers; CaptureDriver cap; drivers.addChild(&cap);
	drivers.setLayers(&layers);
	layers.onBuildState(); drivers.onBuildState();
	REQUIRE(cap.src_ != nullptr); // seed a previously-published source
	only.setEnabled(false);
	layers.onBuildState(); drivers.onBuildState();
	CHECK(layers.activeLayer() == &only); // fallback for geometry
	CHECK(layers.firstEnabledLayer() == nullptr);// no enabled source
	CHECK(layers.enabledLayerCount() == 0);
	CHECK(drivers.dynamicBytes() == 0); // no output buffer allocated
	CHECK(cap.src_ == nullptr); // driver idle — no stale frame
	}

🧰 Tools

🪛 Clang (14.0.6)

[note] 322-322: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 322-322: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 322-322: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 323-323: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 323-323: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 323-323: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 324-324: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 324-324: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 324-324: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 325-325: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 325-325: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 325-325: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 326-326: +1, including nesting penalty of 0, nesting level increased to 1

(clang)

---

[note] 326-326: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

---

[note] 326-326: +2, including nesting penalty of 1, nesting level increased to 2

(clang)

🪛 Cppcheck (2.21.0)

[style] 303-303: The function 'addButton' is never used.

(unusedFunction)

---

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

(unusedFunction)

---

[style] 323-323: The function 'setReadOnly' is never used.

(unusedFunction)

---

[style] 314-314: The function 'severity' is never used.

(unusedFunction)

---

[style] 322-322: The function 'loopTimeUs' is never used.

(unusedFunction)

---

[style] 326-326: The function 'publishTiming' is never used.

(unusedFunction)

---

[style] 317-317: The function 'valid' is never used.

(unusedFunction)

🤖 Prompt for AI Agents

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

In `@test/unit/light/unit_Layers_container.cpp` around lines 299 - 326, The
stale-buffer check in the driver test is too weak because it only covers the
already-disabled initial state. Update the scenario around Layers, Drivers, and
CaptureDriver so it first publishes a frame with the layer enabled, then
disables the layer and rebuilds, and only then asserts the source buffer is
cleared. Keep the existing assertions on activeLayer(), firstEnabledLayer(),
enabledLayerCount(), dynamicBytes(), and cap.src_ to verify the transition from
a valid frame to nullptr.

Source: Learnings