Usermod system improvements (static UI support)#5742
Open
willmmiles wants to merge 12 commits into
Open
Conversation
Remove the in-tree example in favor of the out-of-tree example instead. Encourage people to start their project out-of-tree instead of here.
Use SCons dependencies to only run npm if we need it.
Usermods can now provide a cdata.json manifest alongside their source, declaring HTML/JS/CSS assets to be compiled into a gzipped C header at build time. The file schema is described in `tools/cdata.schema.json` These manifests are processed concordantly with SCons build rules, so the build system can naturally track dependencies and rebuild only when a manifest or its source files change. Includes numerous error handling fixes inside `cdata.js`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Removes the static linkage from handleStaticContent() in wled_server.cpp and adds a forward declaration in fcn_declare.h, making it available to usermods that want to serve their own gzip-compressed static assets via the same ETag/cache/gzip mechanism the core pages use. This is intentionally minimal — the full server API refactor (wled::serve namespace, header decomposition) remains deferred. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a _name member (const char*, PROGMEM ok) to the Usermod protected section, with getName(). Field names are intentionally compatible with MoonModules' fork to allow MM usermods to work with minimal changes. For mods that don't pass a name to the constructor, probe addToConfig() with a small stack-local JsonDocument and take the first top-level key as the name. (H/T MoonModules for this idea.) Key differences from MM's approach: - No base class _enabled: there's no practical solution for backwards compatibility with usermods that don't provide their own handling. - Explicit getter rather than direct field access Also adds UsermodManager::lookup(const char*) to find a usermod by name. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Registers GET and POST handlers for /settings/um/*, placed before the
/settings catch-all so the more-specific prefix wins.
`settings_um.htm` is updated to support a per-usermod endpoint, so only
the one usermod's fields appear. The global I2C/SPI bus section is
moved to `/settings/hw/if`. The original `/settings/um` page is
also preserved for backwards compatibility with old UI integrations.
The global settings panel is expanded to generate links to the per-
usermod page.
GET /settings/mod/<name>:
- Looks up the mod by name via UsermodManager::lookup(const char*)
- Returns 404 if the mod isn't found
- Otherwise serves settings_um.htm (JS uses window.location.pathname
to filter the display to the named mod — implemented in next commit)
- Respects the PIN gate by delegating to serveSettings() when locked
Usermods that register server.on("/settings/mod/mymod") during setup()
shadow this catch-all for their specific path via first-registered-wins
ordering.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds incremental, depfile-driven UI generation with usermod manifests, exposes named usermod settings routes and navigation, updates per-usermod configuration saving, and revises usermod documentation toward out-of-tree templates. ChangesUsermod UI integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PlatformIO
participant CdataCLI
participant GeneratedHeaders
participant Firmware
PlatformIO->>CdataCLI: register depfile-driven UI build
CdataCLI->>GeneratedHeaders: generate stale UI and usermod headers
GeneratedHeaders->>Firmware: provide headers before compilation
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 183-198: Update the MyUsermod example to initialize the base
Usermod name through its constructor using MyUsermod::_name, while preserving
the existing static name declaration and registration. Ensure instances have a
valid base name for named lookup and per-usermod settings routes.
In `@pio-scripts/build_ui.py`:
- Around line 45-68: Invalidate cached dependency graphs when directory contents
or manifest membership changes: in pio-scripts/build_ui.py lines 45-68, update
_depfile_structure_current or configuration flow to regenerate the depfile
unconditionally or compare a signature of manifest paths and directory entries;
in tools/cdata.js lines 528-558, make missing declared inputs mark the scan
stale or fail clearly instead of reusing the old header; in tools/cdata-test.js
lines 164-200, add consecutive-scan coverage for added, deleted, and renamed
HTML resources plus manifest-set changes. Include a small software FMEA/test
matrix covering stale UI artifacts, failure modes, mitigations, mtime/clock
causes, and typical WLED builds.
In `@tools/cdata.js`:
- Around line 658-664: Replace the NODE_ENV-based CLI guard around main() in
tools/cdata.js lines 658-664 with require.main === module so direct execution
still runs while imports remain side-effect-free. In tools/cdata-test.js lines
12-20, remove the global NODE_ENV mutation and the production-only child-process
environment override.
- Around line 501-523: Synchronize executable manifest validation with the
schema: in tools/cdata.js lines 501-523, validate operation and spec keys,
required fields, types, enums, and apply documented defaults before generation;
in tools/cdata.schema.json lines 27-80, ensure optional/default fields describe
output the generator can produce, including valid plaintext delimiters; in
tools/cdata-test.js lines 270-299, add coverage for omitted defaults, unknown
nested keys, invalid methods and filters, symbols, and plaintext delimiters.
- Around line 528-540: Update tools/cdata.js in jobInputs to include
package-lock.json for every job; update pio-scripts/build_ui.py in the UI build
dependency setup to use a lockfile digest stamp instead of the node_modules
mtime heuristic, or run npm ci on every UI command, ensuring npm ci precedes UI
and firmware builds; update tools/cdata-test.js to assert every depfile rule
contains package-lock.json.
- Around line 513-523: Constrain manifest paths to the usermod directory: in
tools/cdata.js lines 513-523, validate resolved output, srcDir, and spec.file
paths and reject absolute paths or paths escaping the permitted directory; in
tools/cdata.schema.json lines 32-40, require non-traversing relative srcDir and
output values, and in lines 55-58 constrain source filenames to remain under
srcDir; in tools/cdata-test.js lines 270-299, add rejection coverage for
traversal and absolute paths.
In `@wled00/data/settings_um.htm`:
- Around line 319-325: Update the per-mod save request in the modFilter branch
to send the complete current um configuration object rather than only the
selected modFilter section. Reuse the existing configuration state used by the
page so UsermodManager::readFromConfig() receives all usermod sections and
unrelated settings remain intact.
In `@wled00/um_manager.cpp`:
- Around line 21-24: In UsermodManager::setup, call each usermod’s setup()
before checking getName() and invoking probeNameFromConfig(). Preserve probing
only for usermods whose name remains null after initialization, ensuring
addToConfig() runs after setup-established state and dependencies are available.
In `@wled00/xml.cpp`:
- Around line 219-232: Update the per-mod name handling in the UsermodManager
loop to copy the result of mod->getName() from PROGMEM into a RAM buffer using
the appropriate platform-safe _P helper before scanning it. Preserve the
existing quote/backslash escaping and addUMBtn output, but iterate over the
copied RAM string rather than dereferencing the original pointer.
🪄 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: CHILL
Plan: Pro
Run ID: 007f8d10-673f-45b5-b320-1a57d57febbd
📒 Files selected for processing (17)
.gitignoreAGENTS.mdpio-scripts/build_ui.pypio-scripts/load_usermods.pytools/cdata-test.jstools/cdata.jstools/cdata.schema.jsonusermods/EXAMPLE/library.jsonusermods/EXAMPLE/readme.mdusermods/EXAMPLE/usermod_v2_example.cppusermods/readme.mdwled00/data/settings.htmwled00/data/settings_um.htmwled00/fcn_declare.hwled00/um_manager.cppwled00/wled_server.cppwled00/xml.cpp
💤 Files with no reviewable changes (2)
- usermods/EXAMPLE/library.json
- usermods/EXAMPLE/usermod_v2_example.cpp
Comment on lines
+658
to
+664
| // Don't run the build when imported by the test harness. | ||
| if (process.env.NODE_ENV !== 'test') { | ||
| main().catch((err) => { | ||
| console.error(err); | ||
| process.exitCode = 1; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use module identity—not NODE_ENV—to guard CLI execution.
An ambient NODE_ENV=test makes the build command exit successfully without generating anything. Use require.main === module so imports remain side-effect-free without disabling legitimate CLI builds.
tools/cdata.js#L658-L664: replace the environment check withif (require.main === module).tools/cdata-test.js#L12-L20: remove the global environment mutation and production-only child override.
📍 Affects 2 files
tools/cdata.js#L658-L664(this comment)tools/cdata-test.js#L12-L20
🤖 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 `@tools/cdata.js` around lines 658 - 664, Replace the NODE_ENV-based CLI guard
around main() in tools/cdata.js lines 658-664 with require.main === module so
direct execution still runs while imports remain side-effect-free. In
tools/cdata-test.js lines 12-20, remove the global NODE_ENV mutation and the
production-only child-process environment override.
Comment on lines
+21
to
+24
| void UsermodManager::setup() { | ||
| for (auto mod = DYNARRAY_BEGIN(usermods); mod < DYNARRAY_END(usermods); ++mod) { | ||
| if ((*mod)->getName() == nullptr) (*mod)->probeNameFromConfig(); // set _name from addToConfig() key if not provided by constructor | ||
| (*mod)->setup(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Probe the name only after the usermod is initialized.
probeNameFromConfig() calls the derived addToConfig() before setup(). Usermods that initialize configuration state or dependencies in setup() can crash or generate an invalid name.
Proposed fix
for (auto mod = DYNARRAY_BEGIN(usermods); mod < DYNARRAY_END(usermods); ++mod) {
- if ((*mod)->getName() == nullptr) (*mod)->probeNameFromConfig();
(*mod)->setup();
+ if ((*mod)->getName() == nullptr) (*mod)->probeNameFromConfig();
}As per path instructions, verify weak lifecycle assumptions in apparently generated C++ code.
Also applies to: 118-122
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 22-22: Comparing pointers that point to different objects
(comparePointers)
🤖 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 `@wled00/um_manager.cpp` around lines 21 - 24, In UsermodManager::setup, call
each usermod’s setup() before checking getName() and invoking
probeNameFromConfig(). Preserve probing only for usermods whose name remains
null after initialization, ensuring addToConfig() runs after setup-established
state and dependencies are available.
Source: Path instructions
blazoncek
reviewed
Jul 18, 2026
| const char* key = it->key().c_str(); | ||
| if (!key || !key[0]) return; | ||
| char* buf = new char[strlen(key) + 1]; | ||
| if (buf) { |
Contributor
There was a problem hiding this comment.
Isn't this pointless? You are not using (std::nothrow).
Or did I get nothrow entirely wrong.
Member
Author
There was a problem hiding this comment.
Good catch, nothrow is missing there.
Remove redundant depfile analysis and ensure directory dependencies are handled correctly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The minified/gzipped bytes the build emits depend on the exact versions of clean-css, html-minifier-terser and web-resource-inliner, which are pinned in package-lock.json. An `npm install` that bumps one of them without touching package.json would previously reinstall node_modules (build_ui.py already watches the lock for that) but leave every generated header untouched, so stale output survived. Add package-lock.json alongside package.json in jobInputs so it feeds both the in-node staleness check and the emitted depfile, invalidating all headers when the locked toolchain changes. A missing lock file stays tolerated: isJobStale ignores absent inputs and the build system drops non-existent depfile sources. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A cdata.json is a third-party build input, but its output/srcDir/spec `file` fields were joined onto the usermod directory with no bounds check, so a `..` sequence (or an absolute path) could read from, or write to, anywhere on the build machine -- e.g. output "../../../../etc/x" or file "../../../secret". That is a build-time path-traversal / arbitrary file write, a supply-chain hazard when compiling someone else's usermod. Add resolveWithin(), which resolves a manifest path against a base dir (using path.resolve so absolute paths are caught, not silently reinterpreted) and fails loudly if it escapes. Apply it to output and srcDir (against the usermod folder) and to each spec file (against srcDir). Spec objects and their `file` string are validated first so the resolution has something well-formed to check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Manifest spec fields reached codegen unvalidated, so a bad value became invalid C++ instead of a clear error: - A plaintext spec that omitted the raw-string delimiters emitted a bare `R"..."`, which has no parentheses and does not compile. specToChunk now fills in a safe matching delimiter pair (=====(...)=====) when the spec supplies neither, so plaintext "just works"; manifestToJobs rejects supplying only one of prepend/append (an unbalanced delimiter). - 'name' is emitted verbatim as a C identifier, so a value with spaces or punctuation could break -- or inject into -- the header. It must now match a strict identifier. - 'method' must be one of plaintext/gzip/binary and 'filter', if present, one of minify()'s filters, validated up front with manifest context rather than surfacing as an "Unknown method/filter" throw mid-build. (Note: an omitted 'filter' is fine -- minify()'s default parameter makes it "plain"; it does not reach minify() as undefined.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/cdata.js (2)
168-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
path.joinfor robust path construction.While string concatenation with
"/"usually works in Node.js on all platforms, usingpath.joinis more idiomatic and ensures cross-platform consistency.jobInputsalready correctly usespath.joinfor this same path.💡 Proposed refactor
async function specToChunk(srcDir, s) { - const buf = fs.readFileSync(srcDir + "/" + s.file); + const buf = fs.readFileSync(path.join(srcDir, s.file)); let chunk = `\n// Autogenerated from ${srcDir}/${s.file}, do not edit!!\n`🤖 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 `@tools/cdata.js` around lines 168 - 170, Update specToChunk to construct the input file path with path.join, matching the existing jobInputs approach, and reuse that path for both fs.readFileSync and the autogenerated source comment instead of concatenating srcDir and s.file with "/".
621-622: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve package manifests relative to
__dirname.Currently,
package.jsonandpackage-lock.jsonare resolved relative to the current working directory (process.cwd()). This works correctly when the script is invoked by SCons from the repository root. However, if a developer runs this script manually from a different directory (like a usermod subfolder), the staleness check will look for these files in that working directory, fail to find them, and silently ignore them due to theENOENTcatch inisJobStale.Using
__dirnameensures these files are always accurately monitored.💡 Proposed refactor
function jobInputs(job) { - const files = [__filename, 'package.json', 'package-lock.json']; + const files = [__filename, path.join(__dirname, '..', 'package.json'), path.join(__dirname, '..', 'package-lock.json')]; const dirs = [];🤖 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 `@tools/cdata.js` around lines 621 - 622, Update jobInputs to construct package.json and package-lock.json paths relative to __dirname, while preserving __filename and the existing staleness-check behavior. Use the path utility already available in the file rather than relying on process.cwd().
🤖 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.
Nitpick comments:
In `@tools/cdata.js`:
- Around line 168-170: Update specToChunk to construct the input file path with
path.join, matching the existing jobInputs approach, and reuse that path for
both fs.readFileSync and the autogenerated source comment instead of
concatenating srcDir and s.file with "/".
- Around line 621-622: Update jobInputs to construct package.json and
package-lock.json paths relative to __dirname, while preserving __filename and
the existing staleness-check behavior. Use the path utility already available in
the file rather than relying on process.cwd().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2385046d-5fba-41a4-b336-c0791b6ab575
📒 Files selected for processing (4)
AGENTS.mdpio-scripts/build_ui.pytools/cdata-test.jstools/cdata.js
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
Member
|
I'm not sure about the spec format. Do we really need users to define things like gzip and mini for each page individually? https://github.com/wled/wled-usermod-example/blob/9ccb6b30a8b049de7c493d0aa3a642bdbe8a7ad5/cdata.json#L10 |
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
3 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.
This PR includes a number of improvements to managing usermods and their user interfaces. Specifically, this PR:
cdata.jsto integrate fully with SCons, so the UI is only built when changed.cdata.jsonmanifest in usermods that allows them to submit their own UI pages for minification and zipping.handleStaticContent()to serve their own zipped UI components.Usermod::_namemember from the WLED-MM design./settings/um/module_namefor its own setting page, backed by a compatible default. That URL can be captued byhandleStaticContent()to allow usermods to supply their own custom settings pages.A sample
cdata.jsonmodule is available in the external-ui branch of thewled-usermod-examplerepo.The next step after this is to expand the new
cdata.jsonmanifest to support additional operations to integrate usermod UI fragments directly in to injection points in the main UI files.Summary by CodeRabbit
cdata.jsonmanifests, including multi-header outputs.