Skip to content

load pixel forge tools from json file, improved tools list, bugfixes#5404

Open
DedeHai wants to merge 7 commits intowled:mainfrom
DedeHai:pixelforge_jsonlist
Open

load pixel forge tools from json file, improved tools list, bugfixes#5404
DedeHai wants to merge 7 commits intowled:mainfrom
DedeHai:pixelforge_jsonlist

Conversation

@DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Mar 5, 2026

  • load available external tools from a json file instead of hard-coding them in the htm file
  • 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 in preview grid
  • fixed bug of page not loading properly (out of sync access to tabSw() )

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

    • Dynamic Tools section with loading state; install, open, update, and delete tools from the UI, plus pending-update indicators.
    • Local tool manifest synchronizes with a remote manifest to detect and apply new or updated tools.
  • Improvements

    • Safer URL and HTML handling for displayed content.
    • Device file/memory info refreshes automatically after uploads, installs, or deletions; file list loading uses cache-busting.

- 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() )
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 5, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Replaced 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

Cohort / File(s) Summary
PixelForge UI & tool manager
wled00/data/pixelforge/pixelforge.htm
Replaced hardcoded tool sections with a #tools container and loading state. Added globals (pT, wv, remoteURL, toolsjson) and functions: loadTools(), saveToolsjson(), isNewer(), compTool(), renderTools(), insT(id), and merged local+remote manifests. Handles install/update/delete flows, marks pending updates, encodes image URLs with encodeURI, and refreshes FS/file list/UI after operations.
Client utilities
wled00/data/common.js
Added esc(s) (HTML escaping) and safeUrl(u) (basic http(s) URL whitelist) helpers used by PixelForge for safe innerHTML and external links.
Server FS metadata refresh
wled00/wled_server.cpp
Calls updateFSInfo() after successful uploads and after delete operations in the edit handler so filesystem/memory metadata is refreshed post-change.
File deletion/generalization
wled00/data/pixelforge/pixelforge.htm (same file)
Replaced imgDel() with generic deleteFile(name) which deletes the given name or its .gz variant, refreshes FS and file list, and re-renders tools.
File list fetching
wled00/data/pixelforge/pixelforge.htm
Added cache-busting query param (cb=Date.now()) when requesting /edit?list=/ and ensured fsMem() is invoked after uploads/deletes in multiple code paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • netmindz
  • softhack007
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the three main aspects of the changeset: loading tools from JSON, improving the tools list UI, and addressing several bugs.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can generate a title for your PR based on the changes with custom instructions.

Set the reviews.auto_title_instructions setting to generate a title for your PR based on the changes in the PR with custom instructions.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between b07cd45 and 30968b3.

📒 Files selected for processing (2)
  • wled00/data/pixelforge/pixelforge.htm
  • wled00/wled_server.cpp

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (3)
wled00/data/pixelforge/pixelforge.htm (3)

652-655: ⚠️ Potential issue | 🟠 Major

Update installs can still fetch/upload the old artifact instead of the pending one.

When t.pending exists, download/upload still use t.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 artifact

Also 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 | 🟡 Minor

Use 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 | 🟠 Major

Delete lookup/path handling is still brittle for leading slash and reserved characters.

.gz detection compares raw f.name and 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

📥 Commits

Reviewing files that changed from the base of the PR and between 30968b3 and 34bcf01.

📒 Files selected for processing (2)
  • wled00/data/common.js
  • wled00/data/pixelforge/pixelforge.htm

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
wled00/data/pixelforge/pixelforge.htm (2)

632-632: ⚠️ Potential issue | 🟠 Major

Sanitize manifest values used in inline handlers.

Line 632 and Line 639-640 still inject raw t.file/t.id into onclick code paths. This leaves a residual injection surface even after adding esc/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 in renderTools() is to apply esc/safeUrl, including values used in onclick, while leaving t.desc as 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 | 🟠 Major

Encode 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

📥 Commits

Reviewing files that changed from the base of the PR and between 34bcf01 and cff13fb.

📒 Files selected for processing (1)
  • wled00/data/pixelforge/pixelforge.htm

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
wled00/data/pixelforge/pixelforge.htm (1)

1213-1219: ⚠️ Potential issue | 🟠 Major

Encode delete path parameter to avoid query breakage on special characters.

Line [1219] still interpolates raw name into 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 /upload failures in saveToolsjson() 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

📥 Commits

Reviewing files that changed from the base of the PR and between cff13fb and ec59000.

📒 Files selected for processing (1)
  • wled00/data/pixelforge/pixelforge.htm

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: swap to wled

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can also rename it if there is a clearer name.

@softhack007 softhack007 modified the milestone: 16.0.0 beta Mar 9, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
wled00/data/pixelforge/pixelforge.htm (1)

1245-1245: ⚠️ Potential issue | 🟠 Major

Encode the delete path query value to avoid malformed deletes for reserved characters.

name is interpolated directly into the query on Line 1245. Filenames containing characters like & or # can break the request and hit the wrong path value.

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

📥 Commits

Reviewing files that changed from the base of the PR and between ec59000 and 2497ae1.

📒 Files selected for processing (1)
  • wled00/data/pixelforge/pixelforge.htm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants