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
| Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced static PixelForge UI with a dynamic tool manager: client loads/merges a local and remote pftools.json, renders install/open/update/delete actions, and installs tools by fetching and uploading gz sources. Added URL/HTML helpers and server-side FS info refresh after uploads/deletes to keep memory/file state current. 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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/data/pixelforge/pixelforge.htm (1)
1245-1245:⚠️ Potential issue | 🟠 MajorEncode the delete
pathquery value to avoid malformed deletes for reserved characters.
nameis interpolated directly into the query on Line 1245. Filenames containing characters like&or#can break the request and hit the wrongpathvalue.Minimal fix
-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 1245, The delete request builds a query with an unencoded filename (name) which can break for reserved characters; update the fetch call that uses getURL(`/edit?func=delete&path=/${name}`) to URL-encode the filename portion using encodeURIComponent (e.g. keep the leading slash literal and replace ${name} with ${encodeURIComponent(name)}) so reserved characters like & and # are safely transmitted.
🤖 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 641-657: In renderTools(), stop escaping t.desc so manifest-provided HTML is rendered raw (remove esc(...) around t.desc) and ensure any interpolated plain-text values like t.pending.ver are escaped via esc(...) before inserting into innerHTML; update occurrences where t.pending.ver is used (and any onclick/template interpolations such as in the insT(...) and getURL(...) usage) to call esc(...) so only intended text is escaped while t.desc remains raw HTML. --- Duplicate comments: In `@wled00/data/pixelforge/pixelforge.htm`: - Line 1245: The delete request builds a query with an unencoded filename (name) which can break for reserved characters; update the fetch call that uses getURL(`/edit?func=delete&path=/${name}`) to URL-encode the filename portion using encodeURIComponent (e.g. keep the leading slash literal and replace ${name} with ${encodeURIComponent(name)}) so reserved characters like & and # are safely transmitted. ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e904ff4e-e403-473a-81d4-2c7ab1ad9600
📒 Files selected for processing (1)
wled00/data/pixelforge/pixelforge.htm
Member
| @DedeHai it looks like "esp01 1MB" cannot handle the size increase 😅 150 bytes over the limit Retrieving maximum program size .pio/build/esp01_1m_full_160/firmware.elf Checking size .pio/build/esp01_1m_full_160/firmware.elf Error: The program size (893059 bytes) is greater than maximum allowed (892912 bytes) Advanced Memory Usage is available via "PlatformIO Home > Project Inspect" *** [checkprogsize] Explicit exit, status 1 RAM: [====== ] 57.4% (used 47004 bytes from 81920 bytes) Flash: [==========] 100.0% (used 893059 bytes from 892912 bytes) ========================= [FAILED] Took 122.76 seconds ========================= |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/data/pixelforge/pixelforge.htm (1)
1238-1245:⚠️ Potential issue | 🟠 MajorEncode the filename before putting it into
path=.Line 1245 still sends the raw
nameinside a query string. Filenames containing#,&, or?will truncate or splitpath=, so the delete can fail or hit the wrong file.Minimal fix
-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` around lines 1238 - 1245, The deleteFile function inserts raw filename into the query string which breaks for names with characters like #,&,?; update the fetch call to URL-encode the filename (e.g. use encodeURIComponent(name) or encodeURIComponent(`/${name}`) when building the path query) so getURL(`/edit?func=delete&path=/${encodeURIComponent(name)}`) (or equivalent parameter-building) is used instead of the raw name; ensure the same encoding is applied wherever the path query is constructed in deleteFile before calling fetch.
🤖 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 610-611: The pending object currently only copies ver/url/file/desc which drops manifest fields like wled_min so compatibility checks use stale local data; modify the code paths that set lt.pending (e.g., in the isNewer(rt.ver, lt.ver) branch and the other pending-assignment sites around the mentioned ranges) to assign the full remote manifest (rt) to lt.pending (or a shallow clone of rt) instead of only selected fields, and ensure insT() and any install/update gating use t.pending (the full manifest) for wled_min and other compatibility checks rather than falling back to the local lt object. - Around line 641-660: Escape the remaining plain-text template fields by wrapping t.ver and t.wled_min with the existing esc() helper (and confirm any other plain-text inserts use esc), e.g., replace unescaped ${t.ver} with ${esc(t.ver)} and ${t.wled_min} with ${esc(t.wled_min)} in the pixelforge.htm template; keep t.desc raw and leave safeUrl() usages as-is (also verify t.pending.ver is already escaped with esc()) so all user-controlled text is properly escaped. --- Duplicate comments: In `@wled00/data/pixelforge/pixelforge.htm`: - Around line 1238-1245: The deleteFile function inserts raw filename into the query string which breaks for names with characters like #,&,?; update the fetch call to URL-encode the filename (e.g. use encodeURIComponent(name) or encodeURIComponent(`/${name}`) when building the path query) so getURL(`/edit?func=delete&path=/${encodeURIComponent(name)}`) (or equivalent parameter-building) is used instead of the raw name; ensure the same encoding is applied wherever the path query is constructed in deleteFile before calling fetch. ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c6aec61-d8e4-4275-b1b1-ec3095c6b576
📒 Files selected for processing (1)
wled00/data/pixelforge/pixelforge.htm
Comment on lines +610 to +611
| if (isNewer(rt.ver, lt.ver)) { | ||
| lt.pending = { ver: rt.ver, url: rt.url, file: rt.file, desc: rt.desc }; // add pending update info |
Contributor
There was a problem hiding this comment.
Use the install target’s manifest for compatibility checks.
pending currently drops wled_min, but insT() installs t.pending || t. If a newer tool raises its minimum WLED version, the UI still evaluates compatibility against the stale local entry and can offer Install/Update for an unsupported release. Please carry the full remote manifest in pending and gate install/update actions against that target object; otherwise the compatibility check added in this PR is easy to bypass for updates.
Also applies to: 649-656, 672-685
🤖 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 610 - 611, The pending object currently only copies ver/url/file/desc which drops manifest fields like wled_min so compatibility checks use stale local data; modify the code paths that set lt.pending (e.g., in the isNewer(rt.ver, lt.ver) branch and the other pending-assignment sites around the mentioned ranges) to assign the full remote manifest (rt) to lt.pending (or a shallow clone of rt) instead of only selected fields, and ensure insT() and any install/update gating use t.pending (the full manifest) for wled_min and other compatibility checks rather than falling back to the local lt object. Comment on lines +641 to +660
| <h3>${esc(t.name)} <small style="font-size:10px">v${t.ver}</small></h3> | ||
| ${installed ? `<button class="sml" style="height:40px;" onclick="deleteFile('${esc(t.file)}')">✕</button>` : ''} | ||
| </div> | ||
| ${t.desc} | ||
| <div style="font-size:10px; color:#888;"> | ||
| by ${esc(t.author)} | <a href="${safeUrl(t.source)}" target="_blank">${safeUrl(t.source)}</a> | ||
| </div> | ||
| ${ | ||
| compatible ? | ||
| `<div class="crw"> | ||
| ${installed ? | ||
| `<button class="btn" onclick="window.location.href=getURL('/${esc(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${esc(t.pending.ver)}</button>` | ||
| : '' | ||
| } | ||
| </div>` | ||
| : `<div style="color:#f44;font-size:12px;"> Requires WLED ${t.wled_min} </div>` |
Contributor
There was a problem hiding this comment.
Finish the minimal escaping pass for the remaining plain-text fields.
t.ver and t.wled_min are still interpolated raw into innerHTML. This file already adopted the lightweight esc/safeUrl approach, so these plain-text values should be escaped too while keeping t.desc raw.
Minimal fix
-<h3>${esc(t.name)} <small style="font-size:10px">v${t.ver}</small></h3> +<h3>${esc(t.name)} <small style="font-size:10px">v${esc(t.ver)}</small></h3> ... -: `<div style="color:`#f44`;font-size:12px;"> Requires WLED ${t.wled_min} </div>` +: `<div style="color:`#f44`;font-size:12px;"> Requires WLED ${esc(t.wled_min)} </div>`🤖 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 641 - 660, Escape the remaining plain-text template fields by wrapping t.ver and t.wled_min with the existing esc() helper (and confirm any other plain-text inserts use esc), e.g., replace unescaped ${t.ver} with ${esc(t.ver)} and ${t.wled_min} with ${esc(t.wled_min)} in the pixelforge.htm template; keep t.desc raw and leave safeUrl() usages as-is (also verify t.pending.ver is already escaped with esc()) so all user-controlled text is properly escaped. 
Collaborator Author
| yea, I have noticed in other branches that the 1M esp is struggeling. probably need to remove some features. |
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.
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