Docs site (MkDocs), web-installer split, per-module tests#33
Merged
Conversation
Stand up a rendered documentation site (Material for MkDocs) as the project's front door and separate the three unlike things that lived under docs/: the doc-site source stays, the web installer moves out to a top-level web-installer/, and the transient internal notes stay excluded. Adds behaviour tests for the 24 effects/modifiers that shipped untested — the broken "Tests" doc links were a missing-coverage signal, now closed. Firmware module ? links point at the rendered site; MoonDeck gains a Build Docs card; the three local dev servers use adjacent ports. KPI: 16384lights | PC:465KB | tick:367/229/1/357/48/6/795/176/49/62/515/347/57/3/1/105us(FPS:2724/4366/1000000/2801/20833/166666/1257/5681/20408/16129/1941/2881/17543/333333/1000000/9523) | ESP32:1369KB | tick:4832us(FPS:206) | heap:31523KB | src:166(30966) | test:115(16439) | lizard:113w Core: - I2cScanModule.h: updated the deviceModels.json path reference to web-installer/ (comment only). UI: - app.js: module ? help links now open the rendered docs site (moonmodules.org/projectMM/moonmodules/<path>.html, .md→.html with #anchor preserved) instead of GitHub blob URLs; all 85 registered docPaths verified to resolve to real pages + anchors. - install-picker.js / install-picker-boards.js: updated docs/install/ path references to web-installer/ (comments only). Light domain: - Added behaviour-specific unit tests for 19 effects (Blurz, BouncingBalls, FixedRectangle, FreqMatrix, FreqSaws, GEQ, GEQ3D, Lissajous, Noise2D, NoiseMeter, PaintBrush, Praxis, Random, RubiksCube, Solid, SphereMove, StarField, StarSky, Tetrix) and 5 modifiers (Block, Circle, Mirror, RippleXZ, Transpose) — ~90 new cases pinning real behaviour, audio effects fed via AudioModule simulate + frozen clock with RAII mic-seat guards, plus degenerate-grid no-crash cases enforcing the every-grid-size rule. Scripts / MoonDeck: - New build_docs.py: uv wrapper around mkdocs build/serve (PEP-723 inline deps); --serve self-cleans a stray mkdocs serve and binds :8422 so it coexists with the installer preview. - moondeck_config.json: new "Build Docs Site" card in the PC docs group. - preview_installer.py: renumbered to :8421; moondeck.py / MoonDeck.md document the adjacent 8420 (MoonDeck) / 8421 (installer) / 8422 (docs) scheme. - moondeck.py, moondeck_ui/app.js, check_devices.py, check_firmwares.py, generate_firmwares.py, improv_provision.py, improv_smoke_test.py: updated docs/install/ path references to web-installer/. Tests: - Regenerated docs/tests/unit-tests.md + scenario-tests.md from the new @module tags (resolves the 25 broken effect/modifier→test-doc anchor links). - test/js and test/python: updated docs/install/ → web-installer/ paths (imports + readFileSync join-args). Docs / CI: - New docs site: mkdocs.yml (Material, instant search, pymdownx.emoji for icons, use_directory_urls:false so relative image/source links resolve on both the site and GitHub, pymdownx.snippets pre-enabled, history/backlog excluded), docs/index.md landing page (replaces docs/landing/index.html). - Moved docs/install/ → web-installer/ (git mv, history preserved); deployed URL stays /install/ (release.yml maps web-installer/ → pages/install/); ~104 references swept across scripts, both workflows, checks, tests, and docs. - release.yml: builds the MkDocs site into the Pages root via build_docs.py; docs/** + mkdocs.yml added to paths:; docs/install/ staging → web-installer/. - test.yml: JS-test path filter docs/install/** → web-installer/**. - CLAUDE.md: commit-gate triggers updated for web-installer/ (device-catalog, firmware-list, JS host-test gates). - Fixed 8 stale cross-doc anchors (architecture/Layer/LcdLedDriver/Drivers headings) to match MkDocs' generated slugs. - architecture.md, building.md, performance.md, testing.md, module specs, esp32-s31-coreboard.md: docs/install/ → web-installer/ link updates. - Design record: docs/backlog/docs-system-overhaul.md (the phased plan), docs/backlog/rename-scripts-to-moondeck.md (deferred rename), docs/history/plans/Plan-20260702, .claude/workflows/write-behaviour-tests.js (the test-generation workflow). - .gitignore: /site/ (MkDocs build output). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR renames the installer directory from Changesweb-installer Rename & MkDocs Documentation Site
Estimated code review effort: 4 (Complex) | ~60 minutes Effort/Modifier Behavior Unit Tests
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/app.js (1)
510-527: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFragile
.md→.htmlconversion.
docPath.replace(".md", ".html")replaces only the first literal match. It works today becausedocPathalways contains exactly one.md(at the end, before an optional#anchor), but it's easy to regress silently if a futuredocPathsegment ever contains.mdelsewhere (e.g. a directory name). A suffix-anchored regex would make the intent explicit and be immune to that.♻️ Proposed fix
- help.href = "https://moonmodules.org/projectMM/moonmodules/" + docPath.replace(".md", ".html"); + help.href = "https://moonmodules.org/projectMM/moonmodules/" + docPath.replace(/\.md(?=$|#)/, ".html");🤖 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 510 - 527, The help-link URL building in app.js uses a fragile docPath.replace(".md", ".html") conversion that only replaces the first literal match. Update the URL construction in the docPath/help anchor logic to use a suffix-anchored replacement so only the trailing .md before any `#anchor` is converted, keeping the intent explicit and avoiding accidental changes if docPath ever contains .md elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/workflows/write-behaviour-tests.js:
- Line 57: The generated prompt currently hardcodes a developer-specific
absolute repo path, which will break for other machines and CI. Update the
prompt निर्माण in write-behaviour-tests.js so it derives the repository location
dynamically from the current workspace/root instead of embedding a fixed
/Users/... value. Make the repo reference generic and reusable in the prompt
template so the same workflow works across contributors and runners.
- Around line 93-98: The module-level return in write-behaviour-tests.js is
invalid JavaScript/ESM and prevents the workflow from loading. Remove the
top-level return and either wrap this result-building block in a named function
or export the object directly from the workflow module, using the existing
wrote, failed, and results logic as the source for the returned data.
In @.github/workflows/release.yml:
- Around line 29-36: The release workflow path filter is missing the web
installer directory, so installer-only changes will not trigger the Pages
deploy. Update the path list in the release workflow to include web-installer/**
alongside the existing docs/** and mkdocs.yml entries, using the same trigger
block that already covers src/ui/install-picker*.js.
In `@docs/architecture.md`:
- Line 250: The rendered link text is stale and still says
install/deviceModels.json even though the target now points to
web-installer/deviceModels.json. Update the visible label in
docs/architecture.md to match the current destination, and keep the surrounding
reference to the catalog schema consistent with the existing
deviceModel/deviceModels.json wording.
In `@docs/backlog/docs-system-overhaul.md`:
- Around line 13-67: Add the missing blank lines around the tables and headings
in the docs/backlog/docs-system-overhaul.md content to satisfy markdownlint
MD058 and MD022. Update the affected Markdown sections in place so each table is
separated by a blank line before and after, and each heading has a blank line
below it; check the top fact table plus the later section headings like the ones
under “The core insight,” “The tools,” and “Phased transition” for consistency.
In `@docs/history/plans/Plan-20260702` - Docs site Phase 0 (MkDocs Material).md:
- Around line 13-14: Update the historical note in the plan so it matches what
was actually shipped: the docs build in CI does not use build_docs.py with
--strict, and link/anchor validation remains warn-level in mkdocs.yml. Adjust
the CI wiring description around deploy-pages and build_docs.py to state that CI
fails on nav/build issues, while broken links/anchors are only documented as a
local strict-mode check in scripts/MoonDeck.md.
In `@docs/index.md`:
- Line 5: The link in the docs index uses an absolute /install/ path, which will
break under the /projectMM/ subpath deployment. Update the link target in the
docs index to use the project-prefixed path consistently with the mkdocs
navigation fix, and ensure any other references in this page that point to
/install/ are updated to the same resolved subpath. Keep the change aligned with
the root-cause fix so the same path behavior is used throughout the site.
In `@docs/tests/unit-tests.md`:
- Line 835: The regression note in the unit test docs is written in past-tense,
change-history language instead of present tense. Update the text in the
affected documentation comment to describe the behavior in present tense only,
and remove phrases like “Before the fix” while keeping the same meaning; use the
surrounding regression note wording in docs/tests/unit-tests.md to locate it.
In `@mkdocs.yml`:
- Around line 101-107: The installer links are using a root-relative /install/
path, which skips the deployed /projectMM/ prefix. Update the Web installer
entry in mkdocs.yml and the homepage links in docs/index.md so they point to
/projectMM/install/ instead, keeping the existing installer CTA text and
placement unchanged.
In `@scripts/MoonDeck.md`:
- Around line 157-173: The new build_docs subsection is interrupting the
existing update_module_docs description, causing the unreferenced-screenshots
sentence to read like part of build_docs. Move the “Reports unreferenced
screenshots…” sentence back so it stays with the update_module_docs content and
the MoonDeck.md section boundaries remain correct; use the update_module_docs
and build_docs headings plus the “Reports unreferenced screenshots” sentence to
locate the affected text.
In `@test/unit/light/unit_BlockModifier.cpp`:
- Around line 67-77: The 1x1x1 expectation in BlockModifier survives degenerate
grids is inconsistent with the BlockModifier algorithm used elsewhere in
unit_BlockModifier.cpp. Update the assertions in the TEST_CASE to match the
actual fold/blockSize results for the {1,1,1} case, using the existing fold and
blockSize helpers as the reference points. Keep the degenerate 0x0x0 coverage,
but ensure the expected size values reflect the real logical size growth
produced by mm::BlockModifier.
In `@test/unit/light/unit_FreqMatrixEffect.cpp`:
- Around line 23-176: The new effect tests repeat the same
Layouts/GridLayout/Layer setup sequence in every TEST_CASE, so extract that
boilerplate into a shared helper such as makeGridLayer or buildEffectLayer.
Centralize the repeated steps in the helper using the existing Layouts,
GridLayout, Layer, and FreqMatrixEffect setup pattern, then update each test to
call it with the desired dimensions/channels and add the effect afterward if
needed.
In `@test/unit/light/unit_GEQ3DEffect.cpp`:
- Around line 44-91: Move the active-mic cleanup out of the manual tail call in
GEQ3DEffect test setup by using the same RAII pattern as AudioGuard from the
other unit tests. The issue is that mic.teardown() is only reached after
REQUIRE, so a failed assertion leaves AudioModule::active_ set; fix this by
introducing an automatic guard around mm::AudioModule in the test so teardown
runs in the destructor even on early abort, and remove the explicit teardown
call at the end.
In `@test/unit/light/unit_StarFieldEffect.cpp`:
- Around line 10-50: The test cases in unit_StarFieldEffect.cpp manually reset
the virtual clock with a trailing setTestNowMs(0), which can be skipped if a
REQUIRE fails and leave later tests polluted. Update each TEST_CASE that calls
setTestNowMs(...) to use the same RAII ClockGuard pattern used in
unit_BouncingBallsEffect.cpp, so the clock is restored automatically by scope
exit. Remove the manual end-of-test reset and keep the guard local to each test
body, especially around the StarFieldEffect checks and the other affected
TEST_CASEs in this file.
---
Outside diff comments:
In `@src/ui/app.js`:
- Around line 510-527: The help-link URL building in app.js uses a fragile
docPath.replace(".md", ".html") conversion that only replaces the first literal
match. Update the URL construction in the docPath/help anchor logic to use a
suffix-anchored replacement so only the trailing .md before any `#anchor` is
converted, keeping the intent explicit and avoiding accidental changes if
docPath ever contains .md elsewhere.
🪄 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: b2199448-d1d8-42d5-a99a-b9cb858dc4ee
⛔ Files ignored due to path filters (5)
scripts/build/build_esp32.pyis excluded by!**/build/**scripts/build/generate_firmwares.pyis excluded by!**/build/**scripts/build/improv_provision.pyis excluded by!**/build/**scripts/build/improv_smoke_test.pyis excluded by!**/build/**web-installer/favicon.pngis excluded by!**/*.png
📒 Files selected for processing (85)
.claude/workflows/write-behaviour-tests.js.github/workflows/release.yml.github/workflows/test.yml.gitignoreCLAUDE.mddocs/architecture.mddocs/backlog/docs-system-overhaul.mddocs/backlog/rename-scripts-to-moondeck.mddocs/building.mddocs/history/plans/Plan-20260702 - Docs site Phase 0 (MkDocs Material).mddocs/index.mddocs/landing/index.htmldocs/moonmodules/core/DevicesModule.mddocs/moonmodules/core/I2cScanModule.mddocs/moonmodules/core/NetworkModule.mddocs/moonmodules/core/SystemModule.mddocs/moonmodules/light/BlendMap.mddocs/moonmodules/light/Drivers.mddocs/moonmodules/light/Layers.mddocs/moonmodules/light/Layouts.mddocs/moonmodules/light/drivers/LcdLedDriver.mddocs/moonmodules/light/drivers/NetworkSendDriver.mddocs/moonmodules/light/drivers/ParlioLedDriver.mddocs/moonmodules/light/drivers/RmtLedDriver.mddocs/moonmodules/light/drivers/drivers.mddocs/performance.mddocs/reference/esp32-s31-coreboard.mddocs/testing.mddocs/tests/scenario-tests.mddocs/tests/unit-tests.mdmkdocs.ymlscripts/MoonDeck.mdscripts/check/check_devices.pyscripts/check/check_firmwares.pyscripts/docs/build_docs.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jsscripts/run/preview_installer.pysrc/core/I2cScanModule.hsrc/ui/app.jssrc/ui/install-picker-boards.jssrc/ui/install-picker.jstest/js/config-ops.test.mjstest/js/improv-frame.test.mjstest/js/installer-eth-only.test.mjstest/js/installer-s31-webflash.test.mjstest/python/test_improv_frame.pytest/python/test_installer_manifests.pytest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_MultiplyModifier_pipeline.jsontest/unit/light/unit_BlockModifier.cpptest/unit/light/unit_BlurzEffect.cpptest/unit/light/unit_BouncingBallsEffect.cpptest/unit/light/unit_CircleModifier.cpptest/unit/light/unit_FixedRectangleEffect.cpptest/unit/light/unit_FreqMatrixEffect.cpptest/unit/light/unit_FreqSawsEffect.cpptest/unit/light/unit_GEQ3DEffect.cpptest/unit/light/unit_GEQEffect.cpptest/unit/light/unit_LissajousEffect.cpptest/unit/light/unit_MirrorModifier.cpptest/unit/light/unit_Noise2DEffect.cpptest/unit/light/unit_NoiseMeterEffect.cpptest/unit/light/unit_PaintBrushEffect.cpptest/unit/light/unit_PraxisEffect.cpptest/unit/light/unit_RandomEffect.cpptest/unit/light/unit_RippleXZModifier.cpptest/unit/light/unit_RubiksCubeEffect.cpptest/unit/light/unit_SolidEffect.cpptest/unit/light/unit_SphereMoveEffect.cpptest/unit/light/unit_StarFieldEffect.cpptest/unit/light/unit_StarSkyEffect.cpptest/unit/light/unit_TetrixEffect.cpptest/unit/light/unit_TransposeModifier.cppweb-installer/README.mdweb-installer/config-ops.jsweb-installer/deviceModels.jsonweb-installer/devices.jsweb-installer/firmwares.jsonweb-installer/improv-frame.jsweb-installer/index.htmlweb-installer/install-orchestrator.jsweb-installer/install.cssweb-installer/install.js
💤 Files with no reviewable changes (1)
- docs/landing/index.html
| agent( | ||
| `Write a behaviour-specific doctest unit test for the projectMM module **${cls}** (${kind}). | ||
|
|
||
| Repo: /Users/ewoud/Developer/GitHub/MoonModules/projectMM |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Hardcoded developer-specific absolute path in generated prompt.
Repo: /Users/ewoud/Developer/GitHub/MoonModules/projectMM bakes one developer's local machine path into the agent prompt. This workflow will produce broken/misleading prompts for any other contributor or CI runner that isn't at that exact path.
🔧 Proposed fix
-Repo: /Users/ewoud/Developer/GitHub/MoonModules/projectMM
+Repo: ${process.cwd()}📝 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
| Repo: /Users/ewoud/Developer/GitHub/MoonModules/projectMM | |
| Repo: ${process.cwd()} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/workflows/write-behaviour-tests.js at line 57, The generated prompt
currently hardcodes a developer-specific absolute repo path, which will break
for other machines and CI. Update the prompt निर्माण in write-behaviour-tests.js
so it derives the repository location dynamically from the current
workspace/root instead of embedding a fixed /Users/... value. Make the repo
reference generic and reusable in the prompt template so the same workflow works
across contributors and runners.
Comment on lines
+93
to
+98
| return { | ||
| written: wrote.length, | ||
| failed: failed.map(r => r ? r.module : 'unknown'), | ||
| audio_or_caveats: results.filter(r => r && r.notes).map(r => ({ module: r.module, notes: r.notes })), | ||
| summary: results.filter(Boolean).map(r => ({ module: r.module, file: r.test_file, behaviours: r.behaviours_pinned })), | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for other workflow files using the same top-level phase()/agent()/parallel()/return pattern,
# and find the runner that loads/executes .claude/workflows/*.js to see if it wraps the module body.
fd . .claude/workflows --type f
rg -n 'return \{' .claude/workflows -A2
rg -nP '\bnew Function\b|vm\.(Script|createContext|runInContext)|\bimport\(' .claude -g '*.js' -C3Repository: MoonModules/projectMM
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- outline .claude/workflows/write-behaviour-tests.js ---'
wc -l .claude/workflows/write-behaviour-tests.js
sed -n '1,170p' .claude/workflows/write-behaviour-tests.js
echo
echo '--- search for workflow runner / execution wrapper ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'phase\(|agent\(|parallel\(|write-behaviour-tests|\.claude/workflows|new Function|vm\.(Script|createContext|runInContext)|Function\(' \
. -C 2Repository: MoonModules/projectMM
Length of output: 13910
Remove the top-level return here. This file is parsed as JavaScript/ESM, so return { ... } at module scope is a syntax error and will stop the workflow from loading. Wrap the workflow body in a function or export the result instead.
🧰 Tools
🪛 Biome (2.5.1)
[error] 93-98: Illegal return statement outside of a function
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/workflows/write-behaviour-tests.js around lines 93 - 98, The
module-level return in write-behaviour-tests.js is invalid JavaScript/ESM and
prevents the workflow from loading. Remove the top-level return and either wrap
this result-building block in a named function or export the object directly
from the workflow module, using the existing wrote, failed, and results logic as
the source for the returned data.
Source: Linters/SAST tools
Comment on lines
+29
to
+36
| # The web installer + docs site are served from Pages by the deploy-pages job | ||
| # below; a change to them must trigger a deploy or it never reaches the live site | ||
| # (the eth-only-provisioning fix shipped a commit that didn't auto-deploy because | ||
| # docs/install was missing here). src/ui/install-picker*.js is already covered by src/**. | ||
| - 'docs/install/**' | ||
| - 'docs/landing/**' | ||
| # web-installer was missing here). src/ui/install-picker*.js is already covered by src/**. | ||
| # docs/** covers every page rendered into the Pages root by MkDocs; mkdocs.yml is | ||
| # the site config (nav/theme) — a nav change with no doc change must still redeploy. | ||
| - 'docs/**' | ||
| - 'mkdocs.yml' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B3 -A3 "paths:" .github/workflows/release.yml | rg -n -A15 "on:"
rg -n "web-installer" .github/workflows/release.ymlRepository: MoonModules/projectMM
Length of output: 1369
🏁 Script executed:
#!/bin/bash
cat -n .github/workflows/release.yml | sed -n '1,70p'Repository: MoonModules/projectMM
Length of output: 3530
🏁 Script executed:
#!/bin/bash
git ls-files | rg '^web-installer(/|$)|/web-installer(/|$)'Repository: MoonModules/projectMM
Length of output: 467
Add web-installer/** to the release workflow paths .github/workflows/release.yml still triggers on docs/** and mkdocs.yml, but not on web-installer/**; installer-only changes will skip the Pages deploy.
🤖 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 29 - 36, The release workflow
path filter is missing the web installer directory, so installer-only changes
will not trigger the Pages deploy. Update the path list in the release workflow
to include web-installer/** alongside the existing docs/** and mkdocs.yml
entries, using the same trigger block that already covers
src/ui/install-picker*.js.
Comment on lines
+13
to
+67
| | Fact | Lives in | Sync today | | ||
| |---|---|---| | ||
| | Control name (`"sparking"`) | `.h` (definition) + `.md` spec + test code | `check_specs.py` checks *presence* in `.md`, not accuracy | | ||
| | Control range/default (8000/16000/22050/44100) | `.h` array + `.md` prose | none — hand-copied | | ||
| | Author/attribution | `.h` `// Author:` + `.md` `Origin:` (40+ effects, two formats) | none | | ||
| | Module name (`FireEffect`) | class + `registerType(...)` string + test `@module` + `.md` anchor + `deviceModels.json` | `check_specs`/`check_devices` partial | | ||
| | Architectural fact (buffer persists; AudioModule respects `enabled`) | `.h` comment + module `.md` + `architecture.md` | none | | ||
| - **No structured doc-comments in source.** `.h`/`.cpp` carry rich *inline `//` rationale* but no Doxygen/`///` API blocks. So "move technical detail into the code" is a real move, not a relabel. | ||
|
|
||
| ## The core insight | ||
|
|
||
| Two different problems wear the same "docs too big" coat, and they have **opposite** fixes: | ||
|
|
||
| 1. **Navigation / readability** (end users). Fix = a rendered site with a top-down nav and search. Additive: nothing is deleted, the raw `.md` gets a front door. **Cheap, immediate, low-risk.** | ||
| 2. **Duplication** (developers). Fix = single-source each fact and *generate* the copies (or include them). Subtractive and invasive: it changes where facts live and how they're authored. **Do incrementally, proven per fact-type before rollout.** | ||
|
|
||
| Phase them in that order: the site is the quick win that makes everything else visible; de-duplication is the slow structural win. Do **not** bundle them — a big-bang "new site + moved all facts into code" is the kind of change *Refactor for simplicity* exists to stop. | ||
|
|
||
| ## The tools (all GitHub-Pages-native) | ||
|
|
||
| | Tool | Role | Why it fits here | | ||
| |---|---|---| | ||
| | **Material for MkDocs** | The site: nav tree, instant search, versioning. | PO-named default; the recognised standard (WLED-adjacent projects, FastLED-ecosystem, thousands of firmware projects use it). Ships to Pages via one CI job. `nav:` in `mkdocs.yml` *is* the top-down structure. | | ||
| | **`pymdownx.snippets`** (`--8<--`) | De-dup mechanism #1: pull real source lines into a doc. | Built into Material. A spec can embed the actual `controls_.addX(...)` block or an author comment *from the `.h`* — one source of truth, rendered in two places. No new tool. | | ||
| | **`mkdocs-gen-files` / `mkdocs-macros`** | De-dup mechanism #2: generate whole doc pages from data at build time. | Standard MkDocs plugins. Lets `generate_test_docs.py`-style generation run *inside* the site build instead of committing generated `.md`. Kills the "forgot to regenerate" drift. | | ||
| | **Doxide** | Developer drill-down: parse `.h`/`.cpp` with Tree-sitter → **Markdown** → rendered *in the same Material site*. | This is the ".h/.cpp viewer layered on top" the PO asked for. Unlike classic Doxygen (1998-era HTML, a separate ugly site), Doxide emits Markdown that lives in *our* nav, *our* search, *our* theme — developers drill from a module's user page straight into its annotated source, one site. | | ||
|
|
||
| Rejected: **classic Doxygen HTML** (separate site, dated UI, not integrated), **Sphinx/Breathe** (Python-doc-shaped, heavier, C++ via Breathe is awkward), **Docusaurus/VitePress** (Node toolchain, no C++ story), **mdBook** (great but no C++ integration, weaker search). Material+Doxide is the least bespoke combination that hits all four goals on Pages. | ||
|
|
||
| ## Phased transition | ||
|
|
||
| ### Phase 0 — Stand up the site (no content changes) — *now, ~half a day* | ||
| - Add `mkdocs.yml` with Material, `pymdownx.snippets`, instant search. Author a `nav:` tree that imposes the **top-down end-user order** (see below) over the *existing* files — no file moves, no rewrites. | ||
| - Add a `build-docs` CI job that `mkdocs build`s and publishes to Pages **alongside** the installer (installer already owns `/install/`; docs take `/` or `/docs/`). | ||
| - Add a **landing page** (`docs/index.md`) — the front door end users currently lack: "what is this → install → first light → effects → drill down." | ||
| - **Outcome:** every existing word is now navigable + searchable, zero duplication introduced, zero risk. This alone solves the *"impossible for end users to read"* complaint. | ||
|
|
||
| ### Phase 1 — Restructure nav for the two audiences — *small* | ||
| - Split the nav (not the files, yet) into **User guide** (README intro, getting-started, effect catalog, per-board install) and **Developer/Reference** (architecture, coding-standards, module specs, generated tests, source-drill). `history/` + `backlog/` stay out of the published nav (internal). | ||
| - Move the *most* end-user-hostile prose (deep architecture) below a "Developer" fold so a user's top-down path never hits it unless they drill. | ||
| - **Outcome:** the "top-to-down, easy navigate" structure, still additive. | ||
|
|
||
| ### Phase 2 — Fold generated tests into the site + surface them to users — *small, high-value* | ||
| - Run the existing test-doc generation *at site-build time* via `mkdocs-gen-files` (stop committing `docs/tests/*.md` — the 25K generated words leave the repo, generated fresh each build). Kills the "forgot to regenerate" drift class entirely. | ||
| - On **each effect/module user page**, auto-embed "Tests proving this works: …" from the same test metadata — so an end user reading about *Fire* sees the tests that pin it. This is the PO's *"github issues will be solved adding a new test to proof it, this should be visible to end users."* The link from issue → test → visible-on-the-module-page becomes the norm. | ||
| - **Outcome:** tests visible to users; 25K words of committed generation deleted (subtraction). | ||
|
|
||
| ### Phase 3 — De-duplicate facts, one fact-type at a time — *incremental, the slow win* | ||
| Prove each on **one module**, then sweep. Order by leverage: | ||
| 1. **Author/attribution** → single source in the `.h`, `--8<--` snippet-include into the `.md`. Deletes 40+ hand-maintained `Origin:` copies. (Lowest risk: it's a comment.) | ||
| 2. **Control names + ranges/defaults** → the `.h` `controls_.addX(...)` block is already the source of truth; either snippet-include it, or extend `check_specs.py` into a *generator* that emits the control table into the spec. Deletes the hand-copied range prose; upgrades `check_specs` from "checks presence" to "owns the table." | ||
| 3. **Cross-doc architectural facts** → pick the one true home (usually `architecture.md` or the `.h`), replace the other copies with a link/anchor per *Document a thing once, reference it generically* (already a principle — this enforces it mechanically). | ||
| - **Outcome:** "change a small thing, many files change" shrinks to "change the source, the copies regenerate." | ||
|
|
||
| ### Phase 4 — Developer drill-down into source — *medium, do last* |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Markdownlint: missing blank lines around tables/headings.
Static analysis flags MD058 (tables not surrounded by blank lines) at lines 13 and 19, and MD022 (headings need a blank line below) at lines 44, 50, 55, 60, and 67.
🧹 Example fix pattern (apply to each flagged table/heading)
- **Duplication hotspots** (each fact lives in N places, changing one forces the others):
+
| Fact | Lives in | Sync today |
|---|---|---|
@@
| Architectural fact (buffer persists; AudioModule respects `enabled`) | `.h` comment + module `.md` + `architecture.md` | none |
+
- **No structured doc-comments in source.**
@@
### Phase 0 — Stand up the site (no content changes) — *now, ~half a day*
+
- Add `mkdocs.yml` with Material, `pymdownx.snippets`, instant search.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 19-19: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 50-50: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 55-55: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60-60: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 67-67: 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/backlog/docs-system-overhaul.md` around lines 13 - 67, Add the missing
blank lines around the tables and headings in the
docs/backlog/docs-system-overhaul.md content to satisfy markdownlint MD058 and
MD022. Update the affected Markdown sections in place so each table is separated
by a blank line before and after, and each heading has a blank line below it;
check the top fact table plus the later section headings like the ones under
“The core insight,” “The tools,” and “Phased transition” for consistency.
Source: Linters/SAST tools
Comment on lines
+67
to
+77
| TEST_CASE("BlockModifier survives degenerate grids") { | ||
| mm::BlockModifier b; | ||
| // 1x1x1: single light is the centre -> distance 0; logical box {1,1,1}. | ||
| CHECK(fold(b, 0, 0, 0, {1, 1, 1}) == mm::Coord3D{0, 0, 0}); | ||
| CHECK(blockSize(b, {1, 1, 1}) == mm::Coord3D{1, 1, 1}); | ||
| // 0x0x0: no crash; the fold and size are still finite (each axis grows by one). | ||
| CHECK_NOTHROW(fold(b, 0, 0, 0, {0, 0, 0})); | ||
| CHECK(blockSize(b, {0, 0, 0}).x >= 1); | ||
| CHECK(blockSize(b, {0, 0, 0}).y >= 1); | ||
| CHECK(blockSize(b, {0, 0, 0}).z >= 1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Wrong expected value for the 1×1×1 degenerate case.
Applying BlockModifier's actual formula (validated by the other three TEST_CASEs in this file) to box={1,1,1}: centerX = (1+1)/2-1 = 0, dx = |1-0| = 1, dy = 1, distance = 1, so the logical size becomes {0,1,0} then each axis grows by one → {1,2,1}, not {1,1,1}. This assertion will fail against the real implementation.
🐛 Proposed fix
- // 1x1x1: single light is the centre -> distance 0; logical box {1,1,1}.
+ // 1x1x1: centre offset (1-0)=1 on each axis -> distance 1; logical box {1,2,1}.
CHECK(fold(b, 0, 0, 0, {1, 1, 1}) == mm::Coord3D{0, 0, 0});
- CHECK(blockSize(b, {1, 1, 1}) == mm::Coord3D{1, 1, 1});
+ CHECK(blockSize(b, {1, 1, 1}) == mm::Coord3D{1, 2, 1});As per path instructions, "Verify tests cover edge cases and match the specifications in docs/moonmodules/" — this edge case doesn't match the modifier's own algorithm.
📝 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
| TEST_CASE("BlockModifier survives degenerate grids") { | |
| mm::BlockModifier b; | |
| // 1x1x1: single light is the centre -> distance 0; logical box {1,1,1}. | |
| CHECK(fold(b, 0, 0, 0, {1, 1, 1}) == mm::Coord3D{0, 0, 0}); | |
| CHECK(blockSize(b, {1, 1, 1}) == mm::Coord3D{1, 1, 1}); | |
| // 0x0x0: no crash; the fold and size are still finite (each axis grows by one). | |
| CHECK_NOTHROW(fold(b, 0, 0, 0, {0, 0, 0})); | |
| CHECK(blockSize(b, {0, 0, 0}).x >= 1); | |
| CHECK(blockSize(b, {0, 0, 0}).y >= 1); | |
| CHECK(blockSize(b, {0, 0, 0}).z >= 1); | |
| } | |
| TEST_CASE("BlockModifier survives degenerate grids") { | |
| mm::BlockModifier b; | |
| // 1x1x1: centre offset (1-0)=1 on each axis -> distance 1; logical box {1,2,1}. | |
| CHECK(fold(b, 0, 0, 0, {1, 1, 1}) == mm::Coord3D{0, 0, 0}); | |
| CHECK(blockSize(b, {1, 1, 1}) == mm::Coord3D{1, 2, 1}); | |
| // 0x0x0: no crash; the fold and size are still finite (each axis grows by one). | |
| CHECK_NOTHROW(fold(b, 0, 0, 0, {0, 0, 0})); | |
| CHECK(blockSize(b, {0, 0, 0}).x >= 1); | |
| CHECK(blockSize(b, {0, 0, 0}).y >= 1); | |
| CHECK(blockSize(b, {0, 0, 0}).z >= 1); | |
| } |
🤖 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_BlockModifier.cpp` around lines 67 - 77, The 1x1x1
expectation in BlockModifier survives degenerate grids is inconsistent with the
BlockModifier algorithm used elsewhere in unit_BlockModifier.cpp. Update the
assertions in the TEST_CASE to match the actual fold/blockSize results for the
{1,1,1} case, using the existing fold and blockSize helpers as the reference
points. Keep the degenerate 0x0x0 coverage, but ensure the expected size values
reflect the real logical size growth produced by mm::BlockModifier.
Source: Path instructions
Generate the test-inventory pages at MkDocs build time (no longer committed) and render the effect/modifier/layout/driver catalogs as MoonLight-style 4-column tables, both from a build hook — the source .md stays authored as readable prose. Consolidates the three LED-output drivers into one card, moves the firmware ? help links onto the rendered docs site, and processes the CodeRabbit batch. Net subtraction: the ~25K-word committed test docs leave the repo (rebuilt from source every build).
KPI: 16384lights | PC:465KB | tick:351/247/1/349/43/6/788/173/50/57/484/337/48/3/1/99us(FPS:2849/4048/1000000/2865/23255/166666/1269/5780/20000/17543/2066/2967/20833/333333/1000000/10101) | ESP32:1369KB | tick:4897us(FPS:204) | heap:31526KB | src:166(30966) | test:115(16430) | lizard:113w
UI:
- app.js: module ? help links now open the rendered docs site (moonmodules.org/projectMM/), with a suffix-anchored .md→.html conversion so a trailing #anchor is preserved (🐇 CodeRabbit).
Scripts / MoonDeck:
- New mkdocs_hooks.py: the docs build hook — generates tests/{unit,scenario}-tests.md into the virtual tree from the test files (never committed → can't drift), renders each catalog page's prose ### blocks as a 4-column table (name+description / preview / parameters / tests+source+detail links), and rewrites out-of-docs source links (../src/*.h, ../CLAUDE.md) to GitHub URLs so they resolve on the served/deployed site. Multi-anchor support (merged cards keep every old anchor), Detail:-line routing to the Links column, and a `⌄ details` link to per-module overflow sections.
- build_docs.py: --serve self-cleans a stray mkdocs serve, binds :8422 (adjacent to MoonDeck :8420 / installer :8421), prints a closing URL summary, and stages the installer into the site on serve so the local preview mirrors production single-origin.
- moondeck.py / moondeck_config.json: "Preview Docs Site" card (long-running serve); a card can now declare fixed `args`.
- check_specs.py: catalog pages validate their per-block `source [..]` links instead of requiring a `## Source` section (removed as redundant with the table's Links column).
Tests:
- unit_StarFieldEffect / unit_GEQ3DEffect: RAII guards (ClockGuard / AudioGuard) so a failed REQUIRE can't leak the frozen clock or the active-mic seat into later tests (🐇). unit_FreqMatrixEffect: extracted the repeated Layouts/Grid/Layer setup into a GridLayer helper (🐇). unit_PinwheelModifier: dropped a past-tense "before the fix" note (🐇).
- Deleted the committed docs/tests/{unit,scenario}-tests.md — now build-generated.
Docs / CI:
- mkdocs.yml: moon-man favicon + logo, extra.css for the catalog tables (table-layout:fixed column widths, column-filling preview images, purple names / grey descriptions), build hook, history/ + backlog/ built into the site but off-nav so internal doc links resolve.
- New docs/assets/{extra.css,favicon.png}.
- drivers.md: rewrote as compact cards — merged the 3 LED-output drivers (RMT/LCD/Parlio) into one card + a comparison-table details section (all three original anchors preserved), moved UI screenshots to the Preview column, trimmed descriptions, routed Detail: links to the Links column.
- effects/modifiers/layouts.md: removed the redundant trailing `## Source` list (each row links its source).
- index.md / mkdocs.yml: /install/ links use the /projectMM/ subpath (🐇). architecture.md: fixed a stale install/ link label (🐇). release.yml: web-installer/** added to the Pages-deploy paths (🐇). Plan-20260702 Phase-0: corrected the CI --strict note to match what shipped (🐇).
- New docs/history/plans/Plan-20260702 - Docs Phase 1+2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
|
@coderabbitai, I asked for review but it is not scheduled in, so again: please review 49c16cb and prosent your findings in a copy pastable prompt for agents |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Owner
|
@coderabbitai, I asked for review but it is not scheduled in, so again: please review 49c16cb and prosent your findings in a copy pastable prompt for agents |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Owner
|
@coderabbitai, I asked for review but it is not scheduled in, so again: please review 49c16cb and prosent your findings in a copy pastable prompt for agents |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
Add spec-drift validation (control ranges + author URLs checked .h↔.md), embed real source into docs via pymdownx.snippets where the doc would otherwise hand-copy it, minimize the module docs to "only what the code can't say", and write the documentation model down in coding-standards.md. The docs-overhaul goal — the code is the doc basis, human docs carry only cross-file knowledge — is now a stated standard, not just a practice. KPI: 16384lights | PC:465KB | tick:346/233/1/332/43/6/783/182/49/59/503/339/48/3/1/95us(FPS:2890/4291/1000000/3012/23255/166666/1277/5494/20408/16949/1988/2949/20833/333333/1000000/10526) | ESP32:1369KB | tick:6134us(FPS:163) | heap:31527KB | src:166(30970) | test:115(16430) | lizard:113w (ESP32 tick/FPS is an idle-monitor capture, not this change — the only src edits are comment-only --8<-- markers; no hot-path code touched.) Core: - ImprovFrame.h / PreviewDriver.h: `// --8<-- [start/end:...]` marker comments (compiler-inert) delimiting the frame-constants enum and the preview wire-format, so the docs embed the real source instead of hand-copying it. Scripts / MoonDeck: - check_specs.py: two new drift guards inside the spec-check gate — a control's numeric range from the .h validated against the doc's prose (block-scoped on catalog pages so a shared name like fps/fadeRate matches only its own module; tolerant of 1–8 / 1-8 / 1 to 8 spellings), and the .h `// Author:` URL validated present in the doc. Plus a card-backed-detail-page exemption for `## Source` (the driver detail pages link back to their catalog card, which carries the source). Tests: - New test/python/test_check_specs_drift.py (8 cases) pins both drift guards: catches a conflicting range / missing URL, stays silent on the common no-range-in-prose and hyphen/en-dash/"to" cases. Docs / CI: - coding-standards.md § Documentation model (new): the settled doc standard — two module kinds (catalog module types: effects/modifiers/layouts/drivers, a library of interchangeable implementations documented as catalog cards; vs services & infrastructure: core *Modules + light infrastructure, standalone pages), two reader surfaces, generated-not-committed, drift-validation, source-as-spec via snippets, external-prior-art-only. Autodoc/Doxide marked TBD (Phase 4b). CLAUDE.md § Documentation updated to point at it (replacing the stale per-module-spec text). - Phase 4a snippets: ImprovProvisioningModule.md embeds ImprovFrame.h:frame-constants; PreviewDriver.md embeds PreviewDriver.h:wire-format. - Docs minimization: PreviewDriver.md 64→31 lines (cross-file-only); the 5 other driver detail pages shrunk to the same template (prior-art/source/tests moved to their drivers.md cards, which gained Origin + Tests + .h + detail links); em-dash→colon in the 6 driver opening lines (coding-standards ban). - Removed all self-referential projectMM v1/v2 prior-art across 13 module docs (superseded prototypes of this repo, not lineage); external prior art (FastLED/WLED/MoonLight) kept. - drivers.md: Links column compacted (per-driver Tests · .h · detail); testing.md documents the new drift checks; backlog + Phase-3/4a plans record the pivots (snippets can't bridge code↔prose → validate instead; Doxide can't use uv → 4a snippets now, 4b next). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every module now has two documentation surfaces: a hand-written summary page (end-user, a 4-column table) and a generated technical page built from the header's /// comments. This lands the working system alongside the existing per-module .md pages — nothing deleted yet — as a committable baseline before the content-optimization sweep. ESP32 build and KPI gates skipped: all src/ changes are inert /// comment additions (desktop builds clean, all tests green), so there is no compiled-code change to catch or perf delta to measure.
Core:
- Control.h: consolidated the old Control.md (type matrix, memory/persistence/design rationale, MoonLight prior art) into /// and ///< comments so the generated page is the complete technical reference; fixed the <textarea>/<value> literal-HTML and e.g.-truncation rendering traps.
- ImprovFrame.h: /// enrichment for its generated page.
Light domain:
- Buffer.h: absorbed Buffer.md (storage, locking, prior art) into the class /// comment.
- PreviewDriver.h: converted the class + per-method // comments to /// (point-list model, wire format, the onBuildState use-after-free coupling) so methods and attributes document on the generated page.
- FireEffect.h / TextEffect.h: minor /// and default touch-ups carried from earlier in the branch.
UI:
- New summary pages (core/ui, core/supporting, light/supporting) rendered by the same ###-block → 4-column-table transform the catalog pages use; supporting rows leave preview/controls blank, Core UI rows carry hand-authored control lists.
Scripts / MoonDeck:
- gen_api.py (new): one Doxygen pass + one moxygen --classes call over all src/{core,light} headers (~7s vs ~150s per-header), writing gitignored per-module pages under docs/moonmodules/{domain}/moxygen/. Private members suppressed via a template denylist; a temporary migration cross-check header links the .h and the original in-site .md; writes only on content change to avoid a mkdocs-serve rebuild loop.
- mkdocs_hooks.py: retarget .h links to the domain-nested generated pages; register the new summary pages for the table transform.
- check_specs.py: exclude generated moxygen/ pages from the spec scan; accept an api/moxygen link in place of a ## Source section.
Docs / CI:
- coding-standards.md § Documentation model: rewritten to the two-surface v2 model (summary + generated), present-tense, crediting the docs.rs/Sphinx-autodoc/Doxygen pattern.
- mkdocs.yml: new "Module summaries" nav section; .gitignore: the generated moxygen/ dirs; release.yml: apt install doxygen for the Pages build.
- Plans: docs-v2 plan saved; the two superseded Phase-4b plans marked/retained as design record.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Owner
|
@coderabbitai, can you do a thorough review of all the commits in this pr and present your findings in a copy pastable prompt for agents? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 33 --repo MoonModules/projectMM --json commits --jq '.commits[] | .oid + " " + .messageHeadline' | head -50
echo "---"
gh pr diff 33 --repo MoonModules/projectMM > /tmp/pr33.diff
wc -l /tmp/pr33.diffLength of output: 840 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
# Extract diffs for key risky files
awk '/^diff --git a\/scripts\/docs\/mkdocs_hooks.py/,/^diff --git a\/[^s]/' pr33.diff | head -400Length of output: 19813 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
grep -n '^diff --git' pr33.diff | nl | sed -n '1,200p'Length of output: 438 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
grep -n '^diff --git' pr33.diff | cat -nLength of output: 13578 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
sed -n '105,232p' pr33.diff
echo "=== gen_api.py ==="
sed -n '5957,6172p' pr33.diffLength of output: 18198 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
sed -n '232,299p' pr33.diff
echo "=== check_specs.py relevant ==="
sed -n '5584,5829p' pr33.diff
echo "=== test_check_specs_drift.py ==="
sed -n '7539,7616p' pr33.diffLength of output: 27280 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
sed -n '6829,6864p' pr33.diff
echo "=== moondeck.py args handling ==="
sed -n '6758,6829p' pr33.diffLength of output: 5654 🤖 web_search executed:
Length of output: 7433
One thing I explicitly verified and can rule out: ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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)
scripts/check/check_specs.py (1)
253-253: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing word boundary:
source \[can match inside "resource [...]".
re.findall(r'source \[[^\]]+\]\(([^)]+)\)', text)isn't anchored, so prose containingresource [label](path)would also match as if it were asource [...]link.🔧 Proposed fix
- src_links = re.findall(r'source \[[^\]]+\]\(([^)]+)\)', text) + src_links = re.findall(r'\bsource \[[^\]]+\]\(([^)]+)\)', text)🤖 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/check/check_specs.py` at line 253, The regex in the link-parsing logic is too permissive and can match “source” inside words like “resource.” Update the pattern used in the `re.findall` call in `check_specs.py` so it requires a word boundary or equivalent anchor before `source`, keeping the existing capture behavior for actual `source [label](path)` links. Keep the fix localized to the link extraction expression near `src_links`.scripts/docs/moxygen-templates/cpp/index.md (1)
1-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPossibly unused template given current
--classes --noindexinvocation.
gen_api.pycalls moxygen with--classes --noindex, and per moxygen's docs that mode renders through the class template only (one file per class).index.md/namespace.mdback the--groups/default single-file output modes, which aren't invoked in this pipeline. Worth confirming these two files are actually exercised (e.g., required by moxygen's template-loader even in--classesmode) versus being unused scaffolding.🤖 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/moxygen-templates/cpp/index.md` around lines 1 - 36, The moxygen C++ index template may be unused in the current `gen_api.py` flow because `moxygen` is invoked with `--classes --noindex`, which typically renders only the class template. Verify whether `index.md` is actually loaded or referenced by the template selection path, and if not, remove it or move its contents into the template(s) used by `--classes` so the docs generation pipeline only keeps exercised templates.docs/history/plans/Plan-20260702 - Docs Phase 4b (source-generated technical docs).md (1)
1-65: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPlan is superseded but lacks the required outcome marker.
This plan's core design — flat
moonmodules/api/<Module>.mdpages (lines 23, 28) — is explicitly superseded by the "Docs v2" plan, which states it "supersedes the earlier Phase-4b 'infra API pages' model." The shippedgen_api.pywrites domain-nestedmoonmodules/{core,light}/moxygen/paths, not this plan'sapi/layout. The sibling filePlan-... Docs Phase 4b (Doxide pilot, attempted, abandoned).mdalready follows the convention of appending a trailing outcome marker; this file should get one too (e.g. "superseded by Docs v2").As per coding guidelines,
docs/history/plans/*.mdmust be "renamed with a trailing outcome marker when the plan ships or is abandoned."🤖 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-20260702` - Docs Phase 4b (source-generated technical docs).md around lines 1 - 65, This plan is superseded and must carry the required trailing outcome marker. Rename the plan file to include a clear status suffix such as “superseded by Docs v2”, matching the convention used by the sibling abandoned plan, and make sure the title/header inside the document reflects that final outcome. Keep the core content unchanged; only update the plan identifier and visible outcome marker so it clearly reads as no longer active.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:
- Line 546: The comment in the release workflow references the wrong generated
docs path, so update the note near the gen_api.py step to match the actual
moxygen output location used by the docs and check_specs references. Make the
comment point to docs/moonmodules/{core,light}/moxygen/ instead of
moonmodules/api/*.md, keeping the wording aligned with the identifiers
gen_api.py and moxygen so future maintainers aren’t misled.
- Around line 543-552: The docs generation step still relies on the runner’s
preinstalled Node/npm for moxygen, so pin the Node version in this workflow by
adding actions/setup-node@v4 before the Doxygen install step. Update the nearby
gen_api.py comment in the release job to point to
moonmodules/{core,light}/moxygen/<Module>.md, and keep the change scoped around
the existing Doxygen/moxygen setup in the workflow.
In `@scripts/check/check_specs.py`:
- Around line 285-289: The moxygen validation in check_specs.py only inspects
the first matching link because re.search stops after one hit and the subsequent
continue skips the rest of the page. Update the link-checking logic around the
api match so it scans every moxygen/*.md link in text, not just the first one,
and validates each stem against _API_STEMS. Keep the existing issue reporting
format in place, but apply it inside a loop or equivalent multi-match search so
later links in ui.md, core/supporting.md, and light/supporting.md are also
checked.
In `@scripts/docs/gen_api.py`:
- Around line 71-86: The Doxyfile built in _doxyfile() writes INPUT as an
unquoted space-joined list, so paths with spaces will be split incorrectly by
Doxygen. Update the INPUT assembly to preserve each header path as a single
argument, using proper quoting/escaping before formatting the INPUT line. Keep
the fix localized to _doxyfile() so generate() continues to receive a valid
Doxyfile even when ROOT or its parents contain spaces.
In `@src/core/Control.h`:
- Around line 221-224: The memory-footprint comment on ControlDescriptor is
stale because the struct now includes fields like aux, hidden, readonly, and
validate, so the 16-byte budget no longer matches the implementation. Update the
documentation near ControlDescriptor in Control.h to reflect the real target for
src/core/**, either by stating the actual footprint goal on ESP32 or by
rewording it as a lean-descriptor intent rather than a fixed 16-byte claim. Keep
the wording aligned with the ControlDescriptor layout and its per-module
fixed-capacity usage.
In `@src/light/drivers/PreviewDriver.h`:
- Around line 30-37: Update the wire-format documentation in PreviewDriver to
state that count is the number of kept points after lattice downsampling, not
ceil(lightCount/stride), and clarify that stride > 1 is determined by the
runtime send-buffer cap rather than only dense grids. Keep the existing
per-frame channel description aligned with the actual preview framing contract
so client implementations using the docs will size buffers correctly.
---
Outside diff comments:
In `@docs/history/plans/Plan-20260702` - Docs Phase 4b (source-generated technical
docs).md:
- Around line 1-65: This plan is superseded and must carry the required trailing
outcome marker. Rename the plan file to include a clear status suffix such as
“superseded by Docs v2”, matching the convention used by the sibling abandoned
plan, and make sure the title/header inside the document reflects that final
outcome. Keep the core content unchanged; only update the plan identifier and
visible outcome marker so it clearly reads as no longer active.
In `@scripts/check/check_specs.py`:
- Line 253: The regex in the link-parsing logic is too permissive and can match
“source” inside words like “resource.” Update the pattern used in the
`re.findall` call in `check_specs.py` so it requires a word boundary or
equivalent anchor before `source`, keeping the existing capture behavior for
actual `source [label](path)` links. Keep the fix localized to the link
extraction expression near `src_links`.
In `@scripts/docs/moxygen-templates/cpp/index.md`:
- Around line 1-36: The moxygen C++ index template may be unused in the current
`gen_api.py` flow because `moxygen` is invoked with `--classes --noindex`, which
typically renders only the class template. Verify whether `index.md` is actually
loaded or referenced by the template selection path, and if not, remove it or
move its contents into the template(s) used by `--classes` so the docs
generation pipeline only keeps exercised templates.
🪄 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: 5f7fc712-3f75-47f6-9caf-1db141696949
📒 Files selected for processing (23)
.github/workflows/release.yml.gitignoredocs/coding-standards.mddocs/history/plans/Plan-20260702 - Docs Phase 4b (Doxide pilot, attempted, abandoned).mddocs/history/plans/Plan-20260702 - Docs Phase 4b (source-generated technical docs).mddocs/history/plans/Plan-20260702 - Docs v2 (two-surface module docs).mddocs/moonmodules/core/supporting/supporting.mddocs/moonmodules/core/ui/ui.mddocs/moonmodules/light/supporting/supporting.mdmkdocs.ymlscripts/check/check_specs.pyscripts/docs/gen_api.pyscripts/docs/mkdocs_hooks.pyscripts/docs/moxygen-templates/cpp/class.mdscripts/docs/moxygen-templates/cpp/index.mdscripts/docs/moxygen-templates/cpp/namespace.mdscripts/docs/moxygen-templates/cpp/page.mdsrc/core/Control.hsrc/core/ImprovFrame.hsrc/light/drivers/PreviewDriver.hsrc/light/effects/FireEffect.hsrc/light/effects/TextEffect.hsrc/light/layers/Buffer.h
Absorbs 19 more modules' hand-written .md content into their .h /// comments so each generated technical page is a complete developer reference, moves the now-redundant per-module .md into archive/ subfolders (kept for cross-check until the Stage-5 switchover deletes them), and processes two rounds of CodeRabbit findings. ESP32 build and KPI gates skipped: every src/ change is an inert /// comment addition (desktop builds clean, all tests green), so there is no compiled-code change to catch or perf delta to measure.
Core:
- 12 modules /// -enriched (AudioModule, DevicesModule, FilesystemModule, FirmwareUpdateModule, HttpServerModule, I2cScanModule, ImprovProvisioningModule, MoonModule, NetworkModule, Scheduler, SystemModule) — each class comment consolidates the old .md's design/memory/persistence/prior-art prose plus per-method /// and per-field ///< detail.
- Control.h: reworded the stale "under 16 bytes" ControlDescriptor footprint to the real lean-descriptor layout (~48 bytes on a 64-bit host) 🐇.
- ImprovProvisioningModule.h: fixed an unbalanced backtick (`connected: <ssid>`) that made Doxygen swallow the class and skip its page.
Light domain:
- 9 modules /// -enriched (Buffer, Layer, Layers, Layouts, Drivers, MappingLUT, BlendMap, EffectBase, ModifierBase).
- PreviewDriver.h: corrected the 0x02 wire-format doc — count is the kept-point total after lattice downsampling, stride>1 driven by the runtime send-buffer cap 🐇.
UI:
- ui/ui.md: added the per-module control lists (Parameters column) to the Core UI summary table; retargeted architecture/gettingstarted/index/catalog links to the generated (developer) or summary (end-user) pages after the archive move, so no active page has a broken link.
- 23 old per-module .md moved to core/archive/ + light/archive/ (nav already points at the summaries).
Scripts / MoonDeck:
- gen_api.py: single Doxygen pass + one moxygen --classes call (~7s vs ~150s per-header); rewrite moxygen's cls_mm-<Class> cross-links to the recombined per-header pages (drops ~8500 broken-link + ~7000 dead-anchor warnings to zero); raise GenApiError instead of silently returning {} when the toolchain is present but fails or under-produces 🐇; quote INPUT paths for spaces 🐇; write-only-on-change to avoid a mkdocs-serve rebuild loop; deterministic (sorted) cross-check original-page lookup.
- mkdocs_hooks.py: _slug() delegates to Python-Markdown's toc slugify instead of a hand-rolled mimic 🐇; register the summary pages for the table transform.
- check_specs.py: recognise a source-generated page as a module's spec (no missing when the .md moved to archive); validate EVERY moxygen link on a page, not just the first 🐇; \b before the `source [` link marker 🐇; exclude archive/ and moxygen/ subtrees.
Tests:
- test_mkdocs_slug.py: pin _slug() against markdown's real slugify + a rendered heading id 🐇.
Docs / CI:
- coding-standards § Documentation model already carries the two-surface v2 model; nav restructured (Light supporting under Effects & building shows, new top-level Core); the superseded Phase-4b plan renamed with its outcome marker 🐇.
- release.yml: pin Node via actions/setup-node@v4 for npx moxygen 🐇.
Reviews:
- 🐇 CI could ship a docs site with zero API pages on a transient npx/npm failure with no red X — fixed: gen_api raises when tools present-but-failing; a page-count floor guards under-production.
- 🐇 setup-node not pinned — added actions/setup-node@v4 (Node 20).
- 🐇 _slug() hand-rolled, drifts from real slugify on unicode — delegated to markdown's slugify + added a pinning test.
- 🐇 check_specs moxygen link check only inspected the first match — now loops all links.
- 🐇 gen_api INPUT unquoted — quoted for space-containing paths.
- 🐇 ControlDescriptor 16-byte comment stale — reworded to the real footprint.
- 🐇 PreviewDriver count/stride wording — corrected to the kept-point / send-buffer-cap contract.
- 🐇 superseded Phase-4b plan lacked an outcome marker — renamed.
- 🐇 check_specs `source` regex could match "resource" — added a word boundary.
- 🐇 moxygen index.md template appeared unused — verified moxygen's loader requires the full template set present (removing it breaks generation); kept with reason.
- 🐇 _module_block substring match / gitignore nested-file coverage — verified safe (bracket/space delimiters; trailing-slash rules cover descendants), no change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/moonmodules/light/effects/effects.md (1)
183-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the
countcap
countis exposed as 1–255, but the effect only processes up toMAX_RIPPLES = 8; values above 8 don’t add more rings. Update the description to mention the 8-ring cap or keep the 1–8 wording.🤖 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/effects/effects.md` at line 183, The `count` documentation in the effects guide is too broad because the implementation only renders up to `MAX_RIPPLES` in the ripple/effects logic, so update the description to reflect the 8-ring cap instead of implying 1–255 are meaningful. Locate the `count` entry in the effects docs and revise it to mention that values above 8 do not add more rings, or change the wording to 1–8 to match the behavior exposed by `MAX_RIPPLES`.scripts/docs/gen_api.py (1)
84-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winQuote
XML_OUTPUTandSTRIP_FROM_PATHtoo.
Both can split on spaces, so leaving them unquoted can break the XML output directory andDefined inpaths.🤖 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/gen_api.py` around lines 84 - 101, Update _doxyfile so XML_OUTPUT and STRIP_FROM_PATH are also quoted like the INPUT paths; both values can contain spaces and should be emitted as single Doxygen arguments. Keep the change localized in _doxyfile and ensure the generated config still sets XML_OUTPUT and STRIP_FROM_PATH correctly without splitting on spaces.test/python/test_mkdocs_slug.py (1)
1-44: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winCI is currently failing:
markdownisn't available when this test runs.Both reported pipeline jobs fail at import:
ModuleNotFoundError: No module named 'markdown'. Theuv run --with pytest --with pyserial pytest test/python -qinvocation doesn't includemarkdown, but this new test needs it to pin_slug()'s slugify delegation. Add--with markdown(or add it to a project dependency group used by the test workflow) wherevertest/pythonis run.🔧 Likely fix (in the CI workflow, not shown in this batch)
-uv run --with pytest --with pyserial pytest test/python -q +uv run --with pytest --with pyserial --with markdown pytest test/python -q🤖 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/python/test_mkdocs_slug.py` around lines 1 - 44, The new slug tests import markdown and fail in CI because the test runner does not install that dependency. Update the test execution setup that runs test/python so markdown is included alongside pytest and pyserial, or move markdown into the dependency group used by that workflow; this affects the new tests around test_slug_matches_markdown_toc_slugify and test_slug_resolves_to_the_real_rendered_heading_id.Source: Pipeline failures
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/HttpServerModule.h`:
- Around line 42-47: The header comment in HttpServerModule.h uses
forward-looking roadmap language that should be rewritten in present tense.
Update the descriptive block around the resumable drain/loop20ms behavior to
keep only current behavior, and remove or rephrase the sentence about the
two-core render/transport split so it no longer reads like a future plan; keep
the focus on the existing HttpServerModule drain path and loop20ms cadence.
In `@src/core/Scheduler.h`:
- Around line 33-36: The core-affinity documentation in Scheduler.h no longer
matches the current scheduler behavior. Update the comment near the Scheduler
and MoonModule definitions so it does not claim per-module task pinning or child
inheritance unless that plumbing exists. If you want to keep the wording, add
the missing affinity field and task creation in Scheduler::tick() / module
setup; otherwise, simplify the doc to describe the current inline single-loop
execution model.
---
Outside diff comments:
In `@docs/moonmodules/light/effects/effects.md`:
- Line 183: The `count` documentation in the effects guide is too broad because
the implementation only renders up to `MAX_RIPPLES` in the ripple/effects logic,
so update the description to reflect the 8-ring cap instead of implying 1–255
are meaningful. Locate the `count` entry in the effects docs and revise it to
mention that values above 8 do not add more rings, or change the wording to 1–8
to match the behavior exposed by `MAX_RIPPLES`.
In `@scripts/docs/gen_api.py`:
- Around line 84-101: Update _doxyfile so XML_OUTPUT and STRIP_FROM_PATH are
also quoted like the INPUT paths; both values can contain spaces and should be
emitted as single Doxygen arguments. Keep the change localized in _doxyfile and
ensure the generated config still sets XML_OUTPUT and STRIP_FROM_PATH correctly
without splitting on spaces.
In `@test/python/test_mkdocs_slug.py`:
- Around line 1-44: The new slug tests import markdown and fail in CI because
the test runner does not install that dependency. Update the test execution
setup that runs test/python so markdown is included alongside pytest and
pyserial, or move markdown into the dependency group used by that workflow; this
affects the new tests around test_slug_matches_markdown_toc_slugify and
test_slug_resolves_to_the_real_rendered_heading_id.
🪄 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: 2fcbacc9-a812-4553-8ac3-612145832b7b
📒 Files selected for processing (64)
.github/workflows/release.ymldocs/architecture.mddocs/backlog/backlog-core.mddocs/gettingstarted.mddocs/history/decisions.mddocs/history/plans/Plan-20260702 - Docs Phase 4b (source-generated technical docs, superseded by Docs v2).mddocs/index.mddocs/moonmodules/core/archive/AudioModule.mddocs/moonmodules/core/archive/Control.mddocs/moonmodules/core/archive/DevicesModule.mddocs/moonmodules/core/archive/FilesystemModule.mddocs/moonmodules/core/archive/FirmwareUpdateModule.mddocs/moonmodules/core/archive/HttpServerModule.mddocs/moonmodules/core/archive/I2cScanModule.mddocs/moonmodules/core/archive/ImprovProvisioningModule.mddocs/moonmodules/core/archive/MoonModule.mddocs/moonmodules/core/archive/NetworkModule.mddocs/moonmodules/core/archive/Scheduler.mddocs/moonmodules/core/archive/SystemModule.mddocs/moonmodules/core/archive/ui.mddocs/moonmodules/core/supporting/supporting.mddocs/moonmodules/light/archive/BlendMap.mddocs/moonmodules/light/archive/Buffer.mddocs/moonmodules/light/archive/Drivers.mddocs/moonmodules/light/archive/EffectBase.mddocs/moonmodules/light/archive/Layer.mddocs/moonmodules/light/archive/Layers.mddocs/moonmodules/light/archive/Layouts.mddocs/moonmodules/light/archive/LightConfig.mddocs/moonmodules/light/archive/MappingLUT.mddocs/moonmodules/light/archive/ModifierBase.mddocs/moonmodules/light/drivers/PreviewDriver.mddocs/moonmodules/light/drivers/RmtLedDriver.mddocs/moonmodules/light/drivers/drivers.mddocs/moonmodules/light/effects/effects.mddocs/moonmodules/light/layouts/layouts.mddocs/moonmodules/light/modifiers/modifiers.mddocs/reference/esp32-s31-coreboard.mdmkdocs.ymlscripts/check/check_specs.pyscripts/docs/gen_api.pyscripts/docs/mkdocs_hooks.pysrc/core/AudioModule.hsrc/core/Control.hsrc/core/DevicesModule.hsrc/core/FilesystemModule.hsrc/core/FirmwareUpdateModule.hsrc/core/HttpServerModule.hsrc/core/I2cScanModule.hsrc/core/ImprovProvisioningModule.hsrc/core/MoonModule.hsrc/core/NetworkModule.hsrc/core/Scheduler.hsrc/core/SystemModule.hsrc/light/drivers/Drivers.hsrc/light/drivers/PreviewDriver.hsrc/light/effects/EffectBase.hsrc/light/layers/BlendMap.hsrc/light/layers/Layer.hsrc/light/layers/Layers.hsrc/light/layers/MappingLUT.hsrc/light/layouts/Layouts.hsrc/light/modifiers/ModifierBase.htest/python/test_mkdocs_slug.py
💤 Files with no reviewable changes (18)
- docs/moonmodules/light/archive/Layouts.md
- docs/moonmodules/core/archive/I2cScanModule.md
- docs/moonmodules/core/archive/DevicesModule.md
- docs/moonmodules/core/archive/ImprovProvisioningModule.md
- docs/moonmodules/light/archive/EffectBase.md
- docs/moonmodules/core/archive/Scheduler.md
- docs/moonmodules/light/archive/BlendMap.md
- docs/moonmodules/light/archive/Layers.md
- docs/moonmodules/light/archive/Buffer.md
- docs/moonmodules/light/archive/MappingLUT.md
- docs/moonmodules/core/archive/MoonModule.md
- docs/moonmodules/core/archive/SystemModule.md
- docs/moonmodules/light/archive/LightConfig.md
- docs/moonmodules/core/archive/Control.md
- docs/moonmodules/light/archive/Layer.md
- docs/moonmodules/core/archive/HttpServerModule.md
- docs/moonmodules/core/archive/NetworkModule.md
- docs/moonmodules/light/archive/Drivers.md
Comment on lines
+42
to
+47
| /// **Hot-path split:** the resumable drain runs on `loop20ms` (the 20 ms transport poll), | ||
| /// deliberately NOT the per-render-tick `loop()`, so pushing preview bytes to the socket is | ||
| /// never charged to the LED render hot path. The LED output is never delayed by the preview; | ||
| /// the preview frame rate is instead bounded by the 20 ms drain cadence, which is the right | ||
| /// trade since the preview is a view and the LEDs are not. When the two-core render/transport | ||
| /// split lands, the drain moves to the transport core and the cadence limit lifts. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Future-tense roadmap note in a source header.
"When the two-core render/transport split lands, the drain moves to the transport core and the cadence limit lifts" is forward-looking/roadmap language. Based on learnings, code/doc comments should stay present-tense except in docs/backlog/, docs/history/, and decisions.md.
✏️ Suggested rephrase (present tense, no roadmap claim)
-/// trade since the preview is a view and the LEDs are not. When the two-core render/transport
-/// split lands, the drain moves to the transport core and the cadence limit lifts.
+/// trade since the preview is a view and the LEDs are not.📝 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
| /// **Hot-path split:** the resumable drain runs on `loop20ms` (the 20 ms transport poll), | |
| /// deliberately NOT the per-render-tick `loop()`, so pushing preview bytes to the socket is | |
| /// never charged to the LED render hot path. The LED output is never delayed by the preview; | |
| /// the preview frame rate is instead bounded by the 20 ms drain cadence, which is the right | |
| /// trade since the preview is a view and the LEDs are not. When the two-core render/transport | |
| /// split lands, the drain moves to the transport core and the cadence limit lifts. | |
| /// **Hot-path split:** the resumable drain runs on `loop20ms` (the 20 ms transport poll), | |
| /// deliberately NOT the per-render-tick `loop()`, so pushing preview bytes to the socket is | |
| /// never charged to the LED render hot path. The LED output is never delayed by the preview; | |
| /// the preview frame rate is instead bounded by the 20 ms drain cadence, which is the right | |
| /// trade since the preview is a view and the LEDs are not. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/HttpServerModule.h` around lines 42 - 47, The header comment in
HttpServerModule.h uses forward-looking roadmap language that should be
rewritten in present tense. Update the descriptive block around the resumable
drain/loop20ms behavior to keep only current behavior, and remove or rephrase
the sentence about the two-core render/transport split so it no longer reads
like a future plan; keep the focus on the existing HttpServerModule drain path
and loop20ms cadence.
Source: Learnings
Comment on lines
+33
to
+36
| /// **Core affinity:** each top-level module declares a core affinity (0 or 1 on ESP32) | ||
| /// and the scheduler pins the module's task to that core. Child modules inherit the | ||
| /// parent's core. On single-core or desktop systems, core affinity is ignored. | ||
| /// |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -i 'coreaffinity|core_affinity|affinity|xTaskCreatePinnedToCore|pinnedToCore' src/core/Scheduler.h src/core/MoonModule.h
fd -a Scheduler.cppRepository: MoonModules/projectMM
Length of output: 537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in src/core/Scheduler.cpp src/core/Scheduler.h src/core/MoonModule.h; do
echo "### $f"
wc -l "$f"
done
echo "### Scheduler.cpp outline"
ast-grep outline src/core/Scheduler.cpp --view expanded
echo "### MoonModule.h outline"
ast-grep outline src/core/MoonModule.h --view expanded
echo "### Relevant lines from Scheduler.cpp"
grep -n -i -E 'coreaffinity|core_affinity|affinity|xTaskCreatePinnedToCore|pinnedToCore|task|modules_|tick|child' src/core/Scheduler.cpp || true
echo "### Relevant lines from MoonModule.h"
grep -n -i -E 'coreaffinity|core_affinity|affinity|task|module|child' src/core/MoonModule.h || trueRepository: MoonModules/projectMM
Length of output: 14867
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### src/core/Scheduler.cpp (lines 60-115)"
sed -n '60,115p' src/core/Scheduler.cpp
echo "### src/core/MoonModule.h (lines 52-76)"
sed -n '52,76p' src/core/MoonModule.hRepository: MoonModules/projectMM
Length of output: 4373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### repo-wide core-affinity/task pinning search"
rg -n -i 'xTaskCreatePinnedToCore|pinnedToCore|core[_ ]?affinity|affinity|setAffinity|task.*core|core.*task' .
echo "### Scheduler/MoonModule declarations"
sed -n '1,120p' src/core/Scheduler.h
sed -n '1,120p' src/core/MoonModule.hRepository: MoonModules/projectMM
Length of output: 29801
Align the core-affinity doc with the current scheduler src/core/Scheduler.h:33-36 describes per-module task pinning, but Scheduler::tick() still runs all top-level modules inline in one loop and MoonModule has no affinity field. Remove that wording or wire up the missing task/affinity plumbing.
🤖 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/Scheduler.h` around lines 33 - 36, The core-affinity documentation
in Scheduler.h no longer matches the current scheduler behavior. Update the
comment near the Scheduler and MoonModule definitions so it does not claim
per-module task pinning or child inheritance unless that plumbing exists. If you
want to keep the wording, add the missing affinity field and task creation in
Scheduler::tick() / module setup; otherwise, simplify the doc to describe the
current inline single-loop execution model.
Collaborator
Author
🐇 CodeRabbit review — dispositionsProcessed all 7 unresolved threads. Verdicts: Fixed:
Skipped (with reason):
Build clean (0 warnings) · all unit tests pass · generated pages regenerated with the two text fixes. |
Processes the pre-merge Reviewer + CodeRabbit findings, fixes the red CI (the slug test's markdown dep), tidies the archived pages' links so mkdocs-serve is warning-free, and records the branch's hard-won docs-generation lessons. All src/ changes are inert /// comment fixes; no compiled-code change.
Core:
- HttpServerModule.h: removed a future-tense "when the two-core split lands" roadmap sentence (present-tense rule) 🐇.
- Scheduler.h + MoonModule.h: corrected the core-affinity comment — the scheduler runs modules inline in one loop with no per-module task or affinity field (the claim was inherited verbatim from the old Scheduler.md and aspirational on main too) 🐇.
Light domain:
- TextEffect.h: the intentional hue-default change (0 -> 128; palette index 0 is black in some palettes) is now pinned by a unit test 👾.
Tests:
- unit_TextEffect: assert the hue default is 128, guarding against a silent regression to black-at-0.
Scripts / MoonDeck:
- (no code change; gen_api/mkdocs_hooks/check_specs unchanged this commit — the review fixes are all in headers + docs.)
Docs / CI:
- .github/workflows/test.yml: add --with markdown to the pytest gate so test_mkdocs_slug.py runs in CI (it imports Python-Markdown's slugify); the test itself now pytest.importorskip's markdown so it skips cleanly for a contributor without it — fixes the red CI collection error.
- architecture.md: line-112 reconciliation — describe the two-surface docs model (summary page + generated technical page) and link coding-standards § Documentation model, so the two docs agree.
- docs/history/decisions.md: carry-forward lessons from the docs overhaul — batch per-item external-tool calls (moxygen 132x -> 1), write-only-on-change to avoid a mkdocs-serve rebuild loop, an unbalanced backtick silently drops a class from Doxygen output, bare <tag> in a /// eats the page, generator must fail loud when present-but-failing, agent enrichment needs an adversarial read not just a build, and scaffolding (archive/banner) is named debt not a resting state.
- moonmodules/{core,light}/archive/*.md: deepened relative links by one ../ level after the archive move (temporary files, deleted at Stage 5) so mkdocs-serve has zero broken-link warnings.
Reviews:
- 👾 Reviewer (Opus, whole-branch): flagged the TextEffect hue default riding in untested — pinned with a test (the one blocker; everything else verified clean, incl. all enriched headers genuinely comment-only).
- 🐇 HttpServerModule future-tense header comment — fixed.
- 🐇 Scheduler core-affinity doc doesn't match code — fixed (reworded to the real inline-loop model).
- 🐇 unit_BlockModifier 1x1x1 expected value "critical" — false positive: {1,1,1} folds to distance 0 -> box {1,1,1}; all 5 cases pass (19/19 assertions). No change.
- 🐇 write-behaviour-tests.js top-level return — false positive: it's a Workflow-tool script (async-wrapped body), Biome mis-parses it as a plain ES module. No change.
- 🐇 write-behaviour-tests.js hardcoded /Users path, release.yml web-installer/** trigger, backlog markdownlint — already fixed/present; no change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
A full documentation-system overhaul: stands up a rendered docs site, de-overloads
docs/, and moves every module's technical documentation into its source header — so the docs are generated from the.h///comments rather than hand-maintained in parallel.mdfiles. Also splits the web installer out ofdocs/and closes the test-coverage gaps the doc links exposed.The branch landed in phases (each was a separately-approved commit); this description reflects the whole PR, not just the initial commit.
Docs site (Phase 0)
use_directory_urls: falseso relative image/source links resolve on both the rendered site and GitHub's raw view.docs/index.mdlanding page; Flash button routes to/install/.release.ymlbuilds the site into the Pages root viascripts/docs/build_docs.py(uv wrapper, PEP-723 inline deps);history/+backlog/excluded (internal).docs/install/→ top-levelweb-installer/docs/(git mv, history preserved). Deployed URL unchanged at/install/(release.yml mapsweb-installer/→pages/install/), so QR codes and OTA URLs keep working. ~104 references swept.Catalog tables + build-time test pages (Phases 1–2)
effects/modifiers/layouts/drivers.md) render from prose###blocks into MoonLight-style 4-column tables (name+description · preview · controls · links) via a build-time hook — the source stays readable prose, the table is generated.docs/tests/*.md) generated from@modulemetadata + per-TEST_CASE//descriptions — gitignored, can't drift.Drift validation + source snippets (Phases 3–4a)
check_specs.pyvalidates (not duplicates) facts shared between.hand docs — a control range or author URL that disagrees fails the check.--8<--snippets embed real source regions (wire formats, enums) so the source is the doc.Docs v2: two-surface module docs (the core of this PR)
Every module now has exactly two documentation surfaces, replacing the old per-module
.md:effects/modifiers/layouts/drivers, or the newcore/ui,core/supporting,light/supporting).moonmodules/{core,light}/moxygen/<Module>.md, produced from the header's///comments by Doxygen → moxygen (the docs.rs / Sphinx-autodoc split). Gitignored, regenerated on build.scripts/docs/gen_api.py— one Doxygen pass + onemoxygen --classescall over everysrc/{core,light}header (~7s vs ~150s per-header), recombined per-header via the XML<location>map. Private members suppressed via a template denylist; raisesGenApiError(not a silent empty site) when the toolchain is present-but-failing; writes only on content change (no mkdocs-serve rebuild loop).///-enriched — each old.md's design/memory/persistence/prior-art prose consolidated into the class///+ per-method////<detail, so the generated page is a complete developer reference..mdmoved toarchive/— kept temporarily for cross-check (each generated page carries a clearly-marked temporary migration banner linking its.h+ archived original); deleted at the Stage-5 switchover.actions/setup-node@v4+apt install doxygenfor the Pages build.Per-module tests (24 new)
[Tests]links were a missing-coverage signal. Added behaviour-specific unit tests for 19 effects + 5 modifiers (~90 cases): audio effects viaAudioModulesimulate + frozen clock; degenerate-grid no-crash cases. Plus a_slug()pinning test and aTextEffecthue-default test.Firmware
?links → rendered site?help links openmoonmodules.org/projectMM/moonmodules/<path>.html(was GitHub blob URLs). These 404 until the site's first Pages deploy (on merge to main); the installer stays live throughout.Verification
Desktop build (0 warnings) · ctest · scenarios · platform boundary · JS + Python host tests · spec-check · docs build with 0 broken links in active pages and 0 in generated pages. Reviewer agent (Opus, whole-branch) verified the enriched headers are genuinely comment-only, the Doxygen/moxygen pipeline is idiomatic, and the Stage-5 cleanup is well-contained.
Follow-ups (not in this PR)
archive/(23 files, ~1566 lines) + strip the temporary cross-check banner. The docs subtraction is already −7312 lines; Stage 5 takes it to ~−8800.scripts/→moondeck/rename — spec'd in backlog, its own commit.?links.🤖 Generated with Claude Code