load pixel forge tools from json file, improved tools list, bugfixes#5404
Open
load pixel forge tools from json file, improved tools list, bugfixes#5404
Conversation
- load available external tools from a json file instead of hard-coding them in - add buttons to update or delete tools (instead of manually deleting files in /edit) - fixed bug that file system memory was not updated after uploading or deleting files - fixed a bug where gifs with a space would not display - fixed bug of page not loading properly (out of sync access to tabSw() )
Contributor
WalkthroughClient-side PixelForge tool management was added (load/merge/install/update/delete with remote manifest sync and dynamic UI). Two client helpers (esc, safeUrl) were added. Server now refreshes filesystem/memory metadata after uploads and deletes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/pixelforge/pixelforge.htm`:
- Around line 1209-1215: The delete logic is brittle: normalize file-list
entries and the target name before comparing for the ".gz" variant and always
URL-encode the path when calling getURL; specifically, when checking fL for gz
use a normalized comparison (trim leading '/' from f.name and compare against
name and `${name}.gz`) and when building the delete request use
encodeURIComponent on the path (e.g., pass `/` + encodeURIComponent(name) or
otherwise ensure the leading slash is included but the filename is encoded) so
files with leading slashes or reserved characters are correctly detected and
deleted (references: variable "name", array "fL", and the fetch call
getURL(`/edit?func=delete&path=/${name}`) ).
- Around line 625-627: The isNewer(vN, vO) function currently uses parseFloat
which miscompares dotted semantic versions (e.g., "1.10" vs "1.9"); change
isNewer to split both version strings on '.' into numeric segments, iterate
corresponding segments converting each to integer (treat missing or non-numeric
segments as 0), compare segment-by-segment returning true as soon as a newer
numeric segment is found, false if an older segment is found, and false if all
compared segments are equal; keep function name isNewer(vN, vO) and ensure it
returns a boolean.
- Around line 633-641: The code currently concatenates untrusted manifest fields
into the HTML string via variable h (t.name, t.desc, t.author, t.source, t.file,
t.id), enabling XSS; instead, stop using template innerHTML concatenation for
manifest data: create DOM elements programmatically (e.g., createElement for
container, h3, description div, author/link div), set user-supplied text with
textContent for t.name/t.desc/t.author and validate/sanitize t.source before
assigning to anchor.href, and replace the inline onclick string that references
deleteFile(...) with an element-specific addEventListener that closes over
t.file/t.id to call deleteFile safely. Ensure URLs are checked (protocol
whitelist) and any IDs/files are validated/escaped before use.
- Around line 601-609: The update flow sets lt.upv to indicate an available
update but later downloads using the old t.url and unconditionally sets t.ver
and clears upv regardless of download/install success; change the sequence so
the code uses the remote artifact URL (rt.url) or updates t.url before
initiating the download, perform the download/install and verify success (HTTP
status/checksum/installer result) and only then assign t.ver and delete lt.upv;
ensure all state mutations (t.url, t.ver, lt.upv) happen after a successful
install and roll back or leave lt.upv intact on failure.
- Around line 394-396: The HTML fragment for the tools block (the <div
id="tools"> containing the inner <div> with text "Loading tools...") uses space
indentation; update the indentation to use tabs instead of spaces for that
modified web-file block and do the same for the other affected block (the
similar fragment around the "Loading tools..." instance near the second
occurrence) so the file complies with the wled00/data web-file indentation
guideline.
In `@wled00/wled_server.cpp`:
- Line 224: The updateFSInfo() call mutates shared state
(fsBytesUsed/fsBytesTotal) and is invoked from concurrent async request
handlers, causing torn reads; protect the update by serializing access—add a
mutex/lock or a critical section around the body of updateFSInfo() (or make
callers post the work to a single execution context) so updates to fsBytesUsed
and fsBytesTotal are atomic; locate updateFSInfo() and its callers (the async
handlers that call updateFSInfo()) and wrap the read/update of
fsBytesUsed/fsBytesTotal in the chosen synchronization primitive to prevent
concurrent writes/reads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2629278b-fbb1-4f59-bca4-fea73cef6259
📒 Files selected for processing (2)
wled00/data/pixelforge/pixelforge.htmwled00/wled_server.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
wled00/data/pixelforge/pixelforge.htm (3)
652-655:⚠️ Potential issue | 🟠 MajorUpdate installs can still fetch/upload the old artifact instead of the pending one.
When
t.pendingexists, download/upload still uset.url/t.file, so an “update” may re-install the current version.🛠️ Use pending source for the install payload
-const f = await fetch(t.url); // url in json must be pointing to a gz file +const src = t.pending || t; +const f = await fetch(src.url); // use pending artifact when updating if (!f.ok) throw new Error("Download failed " + f.status); const fd = new FormData(); -fd.append("data", await f.blob(), t.file + '.gz'); // always use gz for file name (source MUST be gz) +fd.append("data", await f.blob(), src.file + '.gz'); // keep filename aligned with source artifactAlso applies to: 658-665
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/pixelforge/pixelforge.htm` around lines 652 - 655, The install path currently always uses t.url and t.file, causing updates to re-install the current artifact; modify the download/upload logic to prefer the pending artifact when present (use t.pending.url and t.pending.file if t.pending is truthy) for the fetch and FormData append operations (the occurrences that use t.url and fd.append(... t.file ...)), and apply the same change to the second block handling installs (the similar code around lines 658-665) so the pending source is used for the install payload.
616-617:⚠️ Potential issue | 🟡 MinorUse tabs for indentation in modified web-file blocks.
Several changed lines here are space-indented.
As per coding guidelines "
wled00/data/**/*.{htm,html,css,js}: Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data".Also applies to: 622-623, 649-667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/pixelforge/pixelforge.htm` around lines 616 - 617, Replace space indentation with tabs in the modified web-file blocks around the upload code: change the leading spaces before the fd.append("data", new Blob([JSON.stringify(pT)], {type:'application/json'}), toolsjson); and await fetch(getURL("/upload"), { method: "POST", body: fd }); lines (and the other affected blocks referenced near the same pT/Blob/fetch usage) so they use tabs for indentation per the wled00/data web-file guideline.
1212-1214:⚠️ Potential issue | 🟠 MajorDelete lookup/path handling is still brittle for leading slash and reserved characters.
.gzdetection compares rawf.nameand can miss entries with/prefix; delete request path is not URL-encoded.🔧 Minimal robust fix
-name=name.replace('/',''); // remove leading slash if present (just in case) -if (fL.some(f => f.name === `${name}.gz`)) +name = name.replace(/^\//, ''); +if (fL.some(f => f.name.replace(/^\//, '') === `${name}.gz`)) name += '.gz'; // if .gz version of file exists, delete that (handles tools which are stored gzipped on device) ... -const r = await fetch(getURL(`/edit?func=delete&path=/${name}`)); +const r = await fetch(getURL('/edit?func=delete&path=/' + encodeURIComponent(name)));Also applies to: 1218-1218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/pixelforge/pixelforge.htm` around lines 1212 - 1214, Normalize and URL-decode the incoming delete path and consistently strip any leading slash before comparisons: call decodeURIComponent on the incoming name (variable name), then do name = name.replace(/^\/+/, '') to remove leading slashes; when checking for a gzipped counterpart compare against fL entries using the normalized name (i.e., compare f.name === name and f.name === `${name}.gz`), and when issuing the delete request URL-encode the path with encodeURIComponent(name) so reserved characters are handled correctly; update occurrences around the name variable, the fL/f.name checks and the code that appends '.gz' to use this normalized/decoded name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/common.js`:
- Line 132: safeUrl currently only checks the protocol and still allows quote
characters which can break out of an HTML attribute; update safeUrl to (1)
parse/validate the URL scheme is http or https (retain this check in safeUrl),
and (2) ensure the URL contains only safe characters for HTML-attribute context
(reject quotes, whitespace and backticks) or percent-encode/escape such
characters before returning; on any failure return '#' so calls to safeUrl()
never yield a value that can break href="...". Use the safeUrl function name to
locate the change and implement the stricter character whitelist or
parsing+sanitization logic there.
In `@wled00/data/pixelforge/pixelforge.htm`:
- Line 628: The installed-tool check uses substring matching via
f.name.includes(t.file) which can yield false positives; update the logic in the
installed determination (variable installed, iterating fL and f.name vs t.file)
to perform an exact filename match allowing an optional ".gz" suffix (e.g.,
consider f.name equal to t.file or equal to t.file + ".gz", or normalize by
stripping a trailing ".gz" from f.name before comparing) so only true filename
matches are detected.
---
Duplicate comments:
In `@wled00/data/pixelforge/pixelforge.htm`:
- Around line 652-655: The install path currently always uses t.url and t.file,
causing updates to re-install the current artifact; modify the download/upload
logic to prefer the pending artifact when present (use t.pending.url and
t.pending.file if t.pending is truthy) for the fetch and FormData append
operations (the occurrences that use t.url and fd.append(... t.file ...)), and
apply the same change to the second block handling installs (the similar code
around lines 658-665) so the pending source is used for the install payload.
- Around line 616-617: Replace space indentation with tabs in the modified
web-file blocks around the upload code: change the leading spaces before the
fd.append("data", new Blob([JSON.stringify(pT)], {type:'application/json'}),
toolsjson); and await fetch(getURL("/upload"), { method: "POST", body: fd });
lines (and the other affected blocks referenced near the same pT/Blob/fetch
usage) so they use tabs for indentation per the wled00/data web-file guideline.
- Around line 1212-1214: Normalize and URL-decode the incoming delete path and
consistently strip any leading slash before comparisons: call decodeURIComponent
on the incoming name (variable name), then do name = name.replace(/^\/+/, '') to
remove leading slashes; when checking for a gzipped counterpart compare against
fL entries using the normalized name (i.e., compare f.name === name and f.name
=== `${name}.gz`), and when issuing the delete request URL-encode the path with
encodeURIComponent(name) so reserved characters are handled correctly; update
occurrences around the name variable, the fL/f.name checks and the code that
appends '.gz' to use this normalized/decoded name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eea04254-c602-43ba-9129-d05e3151be96
📒 Files selected for processing (2)
wled00/data/common.jswled00/data/pixelforge/pixelforge.htm
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
wled00/data/pixelforge/pixelforge.htm (2)
632-632:⚠️ Potential issue | 🟠 MajorSanitize manifest values used in inline handlers.
Line 632 and Line 639-640 still inject raw
t.file/t.idintoonclickcode paths. This leaves a residual injection surface even after addingesc/safeUrl.🔧 Minimal patch
- ${installed ? `<button class="sml" style="height:40px;" onclick="deleteFile('${t.file}')">✕</button>` : ''} + ${installed ? `<button class="sml" style="height:40px;" onclick="deleteFile('${esc(t.file)}')">✕</button>` : ''} @@ - ${installed ? `<button class="btn" onclick="window.location.href=getURL('/${t.file}')">Open</button>` : `<button class="btn" onclick="insT('${t.id}')">Install</button>`} - ${t.pending && installed ? `<button class="btn" style="color:`#fb2`" onclick="insT('${t.id}')">Update v${t.pending.ver}</button>` : ''} + ${installed ? `<button class="btn" onclick="window.location.href=getURL('/${encodeURIComponent(t.file)}')">Open</button>` : `<button class="btn" onclick="insT('${esc(t.id)}')">Install</button>`} + ${t.pending && installed ? `<button class="btn" style="color:`#fb2`" onclick="insT('${esc(t.id)}')">Update v${t.pending.ver}</button>` : ''}Based on learnings: In
wled00/data/pixelforge/pixelforge.htm, the agreed minimal sanitization inrenderTools()is to applyesc/safeUrl, including values used inonclick, while leavingt.descas raw HTML.Also applies to: 639-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/pixelforge/pixelforge.htm` at line 632, The inline onclick handlers inject raw manifest values (t.file / t.id) into HTML; update renderTools() to pass sanitized values into those handlers—wrap t.file and t.id with the same esc() or safeUrl() helpers used elsewhere so the button HTML uses sanitized strings for deleteFile(...) and any other onclicks at the same block (e.g., the installed button and the id-based handlers on lines ~639-640); ensure you use the exact helper names already in the file and keep t.desc as raw HTML per the agreed approach.
1219-1219:⚠️ Potential issue | 🟠 MajorEncode delete-path filename in query string.
Line 1219 builds the delete URL with raw
name. Filenames containing reserved characters can produce malformed query parameters or wrong paths.🔧 Minimal patch
- const r = await fetch(getURL(`/edit?func=delete&path=/${name}`)); + const r = await fetch(getURL(`/edit?func=delete&path=/${encodeURIComponent(name)}`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/pixelforge/pixelforge.htm` at line 1219, The delete URL is built with an unescaped filename in the fetch call (see the fetch(getURL(`/edit?func=delete&path=/${name}`)) expression), which can break for names with reserved characters; update that call to percent-encode the filename portion using encodeURIComponent (e.g., /${encodeURIComponent(name)}) so the path query parameter is safely encoded before calling fetch/getURL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/data/pixelforge/pixelforge.htm`:
- Line 632: The inline onclick handlers inject raw manifest values (t.file /
t.id) into HTML; update renderTools() to pass sanitized values into those
handlers—wrap t.file and t.id with the same esc() or safeUrl() helpers used
elsewhere so the button HTML uses sanitized strings for deleteFile(...) and any
other onclicks at the same block (e.g., the installed button and the id-based
handlers on lines ~639-640); ensure you use the exact helper names already in
the file and keep t.desc as raw HTML per the agreed approach.
- Line 1219: The delete URL is built with an unescaped filename in the fetch
call (see the fetch(getURL(`/edit?func=delete&path=/${name}`)) expression),
which can break for names with reserved characters; update that call to
percent-encode the filename portion using encodeURIComponent (e.g.,
/${encodeURIComponent(name)}) so the path query parameter is safely encoded
before calling fetch/getURL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5a16781c-7c6e-4770-bcf5-47ba0a00708c
📒 Files selected for processing (1)
wled00/data/pixelforge/pixelforge.htm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/data/pixelforge/pixelforge.htm (1)
1213-1219:⚠️ Potential issue | 🟠 MajorEncode delete path parameter to avoid query breakage on special characters.
Line [1219] still interpolates raw
nameinto the query string. Filenames containing reserved query characters (e.g.&,#,?) can break deletion requests.💡 Minimal fix
async function deleteFile(name){ - name = name.replace('/',''); // remove leading slash if present (just in case) + name = name.replace(/^\//,''); // remove leading slash if present if (fL.some(f => f.name.replace('/','') === `${name}.gz`)) name += '.gz'; // if .gz version of file exists, delete that (handles tools which are stored gzipped on device) if(!confirm(`Delete ${name}?`))return; ovShow(); try{ - const r = await fetch(getURL(`/edit?func=delete&path=/${name}`)); + const r = await fetch(getURL(`/edit?func=delete&path=/` + encodeURIComponent(name))); if(r.ok){🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/pixelforge/pixelforge.htm` around lines 1213 - 1219, The delete request interpolates raw filename into the query string which breaks for names with reserved characters; update the fetch call in the delete flow to URL-encode the filename before insertion. Replace the current template string in the fetch/getURL call (the `/edit?func=delete&path=/${name}` usage) with an encoded value using encodeURIComponent (e.g. `/edit?func=delete&path=/` + encodeURIComponent(name)) so the path parameter is properly escaped; no other logic around name/fL needs changes.
🧹 Nitpick comments (1)
wled00/data/pixelforge/pixelforge.htm (1)
614-618: Handle/uploadfailures insaveToolsjson()instead of failing silently.Right now callers assume persistence succeeded even on 401/500, which can leave local tool metadata out of sync after reload.
💡 Minimal fix
async function saveToolsjson() { const fd = new FormData(); fd.append("data", new Blob([JSON.stringify(pT)], {type:'application/json'}), toolsjson); - await fetch(getURL("/upload"), { method: "POST", body: fd }); + const r = await fetch(getURL("/upload"), { method: "POST", body: fd }); + if (!r.ok) throw new Error("Failed to save " + toolsjson + ": " + r.status); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/pixelforge/pixelforge.htm` around lines 614 - 618, The saveToolsjson function currently POSTs to getURL("/upload") and ignores the response, so callers proceed even on 401/500; update saveToolsjson to await the fetch response, check response.ok (and optionally specific status codes like 401/500), and throw or return a rejected Promise with an informative error message when not ok so callers can detect failures; include the URL/context (getURL("/upload")), the payload identifier (pT/toolsjson), and a short descriptive message in the thrown error or returned rejection so callers can log/handle it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/pixelforge/pixelforge.htm`:
- Around line 456-460: The async function flU can leave fL undefined on
network/parse failures causing renderTools() to crash (e.g., fL.some(...));
update flU to guarantee fL is initialized on all paths: inside the try ensure
you only assign fL after a successful response/JSON parse (check r.ok) and in
the catch (and any non-ok branch) set fL = [] (or an appropriate default array)
so subsequent calls like renderTools() safely operate on an array. Reference:
function flU, variable fL, and renderTools() usage.
---
Duplicate comments:
In `@wled00/data/pixelforge/pixelforge.htm`:
- Around line 1213-1219: The delete request interpolates raw filename into the
query string which breaks for names with reserved characters; update the fetch
call in the delete flow to URL-encode the filename before insertion. Replace the
current template string in the fetch/getURL call (the
`/edit?func=delete&path=/${name}` usage) with an encoded value using
encodeURIComponent (e.g. `/edit?func=delete&path=/` + encodeURIComponent(name))
so the path parameter is properly escaped; no other logic around name/fL needs
changes.
---
Nitpick comments:
In `@wled00/data/pixelforge/pixelforge.htm`:
- Around line 614-618: The saveToolsjson function currently POSTs to
getURL("/upload") and ignores the response, so callers proceed even on 401/500;
update saveToolsjson to await the fetch response, check response.ok (and
optionally specific status codes like 401/500), and throw or return a rejected
Promise with an informative error message when not ok so callers can detect
failures; include the URL/context (getURL("/upload")), the payload identifier
(pT/toolsjson), and a short descriptive message in the thrown error or returned
rejection so callers can log/handle it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 41ff4ec5-0408-44bf-8872-ccfc790f8de1
📒 Files selected for processing (1)
wled00/data/pixelforge/pixelforge.htm
netmindz
reviewed
Mar 6, 2026
| let gF=null,gI=null,aT=null; | ||
| let fL; // file list | ||
| let pT = []; // local tools list from JSON | ||
| const remoteURL = 'https://dedehai.github.io/pf_tools.json'; // Change to your actual repo |
Collaborator
Author
There was a problem hiding this comment.
can also rename it if there is a clearer name.
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.
Pixel Forge tool-list is now much more future-proof and user friendly as adding, updating and removing tools no longer require manually deleting files in the editor.
@netmindz the pf_tools.json containing the list of available tools allows to dynamically manage the tools without any change to source code, so if tools are added in the future, the list automatically updates. Therefore this json file should eventually (i.e. before 0.16 release) be hosted in the WLED repo instead of mine. it requires a github.io page as github does not allow to pull files directly from a repo.
Summary by CodeRabbit
New Features
Improvements