new file editor #4956
new file editor #4956
Conversation
WalkthroughAdds a rebuilt web editor page and its generated header to the build; replaces the client editor UI with an ACE‑based editor and new tree/uploader UI; removes client visibility toggles for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Potential hotspots to review:
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
wled00/data/edit.htm (4)
10-12: Adddefer(and optionally SRI) to CDN script tags to avoid render‑blocking and improve security.These scripts are optional; deferring them prevents blocking HTML parsing. If you keep CDN usage, consider SRI +
crossorigintoo.-<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js"></script> -<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js"></script> -<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js"></script> +<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js" defer></script> +<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js" defer></script> +<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js" defer></script>
136-146: Add XHR timeout/error handlers to prevent a stuck loader.Without timeout/error hooks, the spinner can remain visible indefinitely.
- this.x=new XMLHttpRequest(); - this.x.onreadystatechange=cb(this.x,req); + this.x=new XMLHttpRequest(); + this.x.timeout = 10000; + this.x.onreadystatechange=cb(this.x,req); + this.x.onerror = () => { ge("loader").style.display="none"; this.r=false; alert("Network error"); }; + this.x.ontimeout = () => { ge("loader").style.display="none"; this.r=false; alert("Request timed out"); };
200-204: Don't require a file extension for new files.Extensions aren’t mandatory in WLED data (e.g., user files). Allow any non‑empty name.
- if(!newPath || newPath.indexOf(".") == -1) { - alert("Please specify a filename with extension (e.g., newfile.txt)"); + if(!newPath) { + alert("Please specify a filename");
662-662: Remove unused hidden iframe to save bytes.Downloads use
window.open; the iframe isn’t referenced elsewhere.- <iframe id="download-frame" style="display:none;"></iframe>wled00/wled_server.cpp (2)
331-340: POST /edit path: consider early 401 if PIN is not correct to avoid needless upload callbacks.Minor: you rely on handleUpload() to reject when
!correctPIN. You could short‑circuit in the request handler for clarity and to reduce callback invocations.- server.on(F("/edit"), HTTP_POST, - [](AsyncWebServerRequest *request) { - // Don't send any response here - let the upload callback handle it - DEBUG_PRINTLN("POST handler called - upload should be handled by callback"); - }, + server.on(F("/edit"), HTTP_POST, + [](AsyncWebServerRequest *request) { + if (!correctPIN) { request->send(401, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_unlock_cfg)); return; } + DEBUG_PRINTLN("POST handler called - upload should be handled by callback"); + },
232-235: Remove no‑op parameter loop (dead code).The loop reads params then discards them. Safe to drop.
- for (int i = 0; i < request->params(); i++) { - AsyncWebParameter* p = request->getParam(i); - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tools/cdata.js(2 hunks)wled00/data/edit.htm(1 hunks)wled00/data/index.js(0 hunks)wled00/wled_server.cpp(3 hunks)
💤 Files with no reviewable changes (1)
- wled00/data/index.js
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/wled_server.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
🧠 Learnings (2)
📚 Learning: 2025-09-20T09:00:08.933Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.933Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/wled_server.cpptools/cdata.js
📚 Learning: 2025-09-20T09:00:08.933Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.933Z
Learning: Always commit both the web source files and the generated wled00/html_*.h files
Applied to files:
tools/cdata.js
🧬 Code graph analysis (1)
wled00/wled_server.cpp (1)
tools/cdata.js (2)
i(63-63)output(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (3)
wled00/data/edit.htm (1)
400-431: Confirm ledmap pretty‑printer supports actual ledmap schema(s).If
mapis nested or structured differently, alignment logic may misformat. Verify against typical 1D/2D ledmap files.Would you like me to adapt
prettyLedmap()to detect and format nested arrays if present?tools/cdata.js (2)
29-29: LGTM: added html_edit.h to outputs.This ensures rebuild detection includes the new page header.
252-263: LGTM — generated header missing; verify wled00/html_edit.h contains PAGE_edit symbolswled00/wled_server.cpp:307 references PAGE_edit and tools/cdata.js:257 registers name: "PAGE_edit", but wled00/html_edit.h is not present in the repo — confirm the build step generates this header or commit it.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
wled00/data/edit.htm (4)
246-276: DELETE uses multipart FormData; prefer URL-encoded body to maximize server parsing compatibility.AsyncWebServer reliably parses x-www-form-urlencoded; multipart on DELETE can be flaky across stacks.
Apply this diff:
- function deleteFile(p, tree) { + function deleteFile(p, tree) { if (!confirm("Are you sure you want to delete " + p + "?")) return; - var fd = new FormData(); - fd.append("path", p); - req.add("DELETE", "/edit", fd, function(st, resp) { + var body = "path=" + encodeURIComponent(p); + req.add("DELETE", "/edit", body, function(st, resp) { if (st != 200) alert("ERROR " + st + ": " + resp); else tree.refreshPath("/"); }); }
136-146: Add a request timeout to prevent a stuck loader on network issues.Without a timeout, loader can remain visible indefinitely.
this.x=new XMLHttpRequest(); this.x.onreadystatechange=cb(this.x,req); + this.x.timeout = 10000; // 10s + this.x.ontimeout = () => { + ge("loader").style.display="none"; + this.r = false; + alert("Request timed out"); + };
461-495: Initialize Ace robustly and add live JSON validation parity with textarea.Current 100ms probe may miss deferred CDN load; no live validation in Ace path.
- // Try to initialize Ace editor if available - setTimeout(function() { - if(typeof ace !== 'undefined') { + // Try to initialize Ace editor if available (poll a few times) + let aceTries = 0; + function tryInitAce() { + if (typeof ace !== 'undefined') { try { aceEditor = ace.edit(editorDiv); aceEditor.setTheme("ace/theme/monokai"); aceEditor.session.setMode("ace/mode/text"); aceEditor.setOptions({ fontSize: "13px", fontFamily: "monospace", showPrintMargin: false, wrap: true }); useAce = true; console.log("Ace editor loaded successfully"); // Always switch to Ace if available (even with no file) if(useAce && aceEditor) { switchToAce(); } + // Live JSON validation in Ace (offline-friendly) + aceEditor.session.on("change", function() { + const pathField = ge("upload-path"); + const filename = (pathField && pathField.value) ? pathField.value : currentFile; + if (filename && filename.toLowerCase().endsWith(".json")) { + try { + JSON.parse(aceEditor.getValue()); + editorDiv.style.border = "2px solid #333"; + } catch(e) { + editorDiv.style.border = "2px solid #f00"; + } + } else { + editorDiv.style.border = "2px solid #333"; + } + }); + // Monitor filename input for JSON highlighting var pathField = ge("upload-path"); if(pathField) { pathField.oninput = function() { updateEditorMode(); }; } } catch(e) { console.log("Ace editor failed to load:", e); useAce = false; } - } - }, 100); + return; + } + if (++aceTries < 30) setTimeout(tryInitAce, 100); // retry up to ~3s + } + setTimeout(tryInitAce, 100);
656-663: Remove unused download iframe to save bytes.No longer used after switching to window.open().
<div id="preview"></div> - <iframe id="download-frame" style="display:none;"></iframe>wled00/wled_server.cpp (1)
319-334: Harden DELETE: accept path from query or body; forbid deleting wsec.json.Improves robustness (client can send either) and prevents accidental credential wipes.
- if (request->method() == HTTP_DELETE) { - if (!request->hasParam(FPSTR(s_path), true)) { + if (request->method() == HTTP_DELETE) { + if (!request->hasParam(FPSTR(s_path), true) && !request->hasParam(FPSTR(s_path))) { request->send(400, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_not_found)); return; } - String path = request->getParam(FPSTR(s_path), true)->value(); + String path = request->hasParam(FPSTR(s_path), true) + ? request->getParam(FPSTR(s_path), true)->value() + : request->getParam(FPSTR(s_path))->value(); if (!path.startsWith("/")) path = "/" + path; + // disallow deleting WiFi secrets + if (path.indexOf(FPSTR(s_wsec)) >= 0) { + request->send(403, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_accessdenied)); + return; + } + if (!WLED_FS.remove(path)) { request->send(500, FPSTR(CONTENT_TYPE_PLAIN), F("Delete failed")); return; } request->send(200, FPSTR(CONTENT_TYPE_PLAIN), F("File deleted")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/edit.htm(1 hunks)wled00/wled_server.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/wled_server.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
🧠 Learnings (2)
📚 Learning: 2025-09-20T09:00:08.933Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.933Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-20T09:00:08.933Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.933Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
🧬 Code graph analysis (1)
wled00/wled_server.cpp (2)
tools/cdata.js (2)
path(19-19)output(29-29)wled00/data/common.js (1)
path(85-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (5)
wled00/data/edit.htm (2)
528-543: Textarea JSON validation is good. Mirror the same border behavior for Ace editor (see above).Ensures “validation during editing” claim holds with and without CDN.
14-110: Indentation complies with repo rule (tabs in wled00/data).Thanks for aligning to tabs throughout.
Also applies to: 112-154, 156-184, 186-246, 248-276, 279-398, 400-431, 432-642, 656-663
wled00/wled_server.cpp (3)
296-312: Good: downloading wsec.json is blocked.This closes the secret leakage hole.
770-773: Enabling editor handler immediately after PIN acceptance: LGTM.Fixes the “stay locked until visiting Lock page” issue.
229-336: Overall /edit routing is solid (PIN-gated GET/DELETE, POST via upload callback).Directory listing excludes wsec.json; edit/download paths normalized; default serves editor page with cache headers via handleStaticContent.
Please confirm AsyncWebServer on both ESP8266 and ESP32 parses multipart bodies for DELETE. If not, the paired client refactor (URL-encoded body) plus the server query/body handling above will fully cover it.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
wled00/data/edit.htm (6)
349-356: Optional: Show directories in the tree.Currently only files are listed. If the backend returns
type: "dir", users can't navigate into folders.Apply this minimal enhancement:
function addList(path,items){ for(var i=0;i<items.length;i++){ - if(items[i].type==="file" && items[i].name !== "wsec.json") { // hide wsec.json, this is redundant on purpose (done in C code too), just in case... + if(items[i].type==="file" && items[i].name !== "wsec.json") { // hide wsec.json, this is redundant on purpose (done in C code too), just in case... var fullPath = path === "/" ? "/" + items[i].name : path + "/" + items[i].name; createLeaf(fullPath, items[i].name); + } else if (items[i].type==="dir") { + // Simple folder entry; click to load its contents + (function(dirName){ + var fullPath = path === "/" ? "/" + dirName : path + "/" + dirName; + var folder = ce("div"); + folder.style.cssText="cursor:pointer;padding:2px 4px;border-radius:2px;position:relative;color:#8cf"; + folder.textContent = dirName + "/"; + folder.onclick = function(){ + treeRoot.innerHTML = ""; + httpGet(fullPath); + }; + folder.onmouseover=function(){ folder.style.background="#333"; }; + folder.onmouseout=function(){ folder.style.background=""; }; + treeRoot.appendChild(folder); + })(items[i].name); } } }
165-174: Keep editor mode in sync with filename input.Tie the filename input’s oninput to the editor’s mode switch so JSON highlighting updates as users type.
Apply this diff:
var path = ce("input"); path.id = "upload-path"; path.type = "text"; path.value = ""; path.placeholder = "Filename (e.g. config.json)"; + // Update editor mode when filename changes (if editor exposes updateMode) + path.oninput = function(){ + if (editor && typeof editor.updateMode === "function") editor.updateMode(); + };And expose
updateModeon the editor object (see next comment).
583-624: Expose anupdateMode()method on the editor API.Lets the uploader filename field trigger JSON/text mode updates.
Apply this diff:
var ed={ save:function(){ @@ httpPost(filename, getCurrentContent()); }, loadUrl:function(fn){ @@ httpGet(fn); }, loadNewFile:function(fn, content){ @@ updateEditorMode(); // Update editor mode after loading new file - } + }, + updateMode:function(){ + updateEditorMode(); + } };
447-480: Make Ace initialization robust (don’t rely on a 100ms race).A single 100ms timeout can miss the CDN load. Initialize Ace on load and retry if needed.
Apply this diff:
- // Try to initialize Ace editor if available - setTimeout(function() { - if(typeof ace !== 'undefined') { - try { - aceEditor = ace.edit(editorDiv); - aceEditor.setTheme("ace/theme/monokai"); - aceEditor.session.setMode("ace/mode/text"); - aceEditor.setOptions({ - fontSize: "13px", - fontFamily: "monospace", - showPrintMargin: false, - wrap: true - }); - useAce = true; - console.log("Use Ace editor"); - - // Always switch to Ace if available (even with no file) - if(useAce && aceEditor) { - switchToAce(); - } - - // Monitor filename input for JSON highlighting - var pathField = ge("upload-path"); - if(pathField) { - pathField.oninput = function() { - updateEditorMode(); - }; - } - } catch(e) { - console.log("Ace load failed:", e); - useAce = false; - } - } - }, 100); + function initAce(){ + if (useAce || typeof ace === 'undefined') return; + try { + aceEditor = ace.edit(editorDiv); + aceEditor.setTheme("ace/theme/monokai"); + aceEditor.session.setMode("ace/mode/text"); + aceEditor.setOptions({ fontSize:"13px", fontFamily:"monospace", showPrintMargin:false, wrap:true }); + useAce = true; + console.log("Use Ace editor"); + switchToAce(); + updateEditorMode(); + } catch(e) { + console.log("Ace load failed:", e); + useAce = false; + } + } + // Try now, then on window load as a fallback + setTimeout(initAce, 100); + window.addEventListener('load', initAce);
115-152: Add timeout/error handling to XHR queue to avoid stuck loader and dead requests.Currently no
onerror/ontimeout. The spinner can get stuck and the queue wedged.Apply this diff:
this.x=new XMLHttpRequest(); - this.x.onreadystatechange=cb(this.x,req); + this.x.onreadystatechange=cb(this.x,req); + // Basic timeout and error handling + this.x.timeout = 10000; + var fail = function(msg){ + ge("loader").style.display="none"; + try { req.callback(0, msg); } catch(_) {} + if(that.q.length===0) that.r=false; + if(that.r) that._request(that.q.shift()); + }; + this.x.onerror = function(){ fail("Network error"); }; + this.x.ontimeout = function(){ fail("Request timeout"); };
385-415: Safer width handling for ledmap pretty-printing.If
obj.widthis missing/invalid or doesn’t dividemap.length, current logic can misalign rows.Apply this diff:
- let width = obj.width || obj.map.length; + let width = Number.isInteger(obj.width) && obj.width > 0 ? obj.width : obj.map.length; + if (obj.map.length % width !== 0) width = obj.map.length; // fallback to single row if not rectangular
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/edit.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
🧠 Learnings (1)
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (2)
wled00/data/edit.htm (2)
629-638: Overall wiring LGTM; order is fine given the newupdateMode()hook.With
updateMode()exposed and bound from the uploader, editor mode stays in sync even though editor is created before uploader.Please verify that JSON files still validate/red-border in textarea fallback and Ace’s JSON worker loads when online (check Network tab for
worker-json.min.js).
9-12: Harden ACE script includes: adddefer,crossorigin="anonymous", and SRI
Replace lines 9–12 in wled00/data/edit.htm:-<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js"></script> -<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js"></script> -<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js"></script> +<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js" defer crossorigin="anonymous" integrity="sha384/…"></script> +<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js" defer crossorigin="anonymous" integrity="sha384/…"></script> +<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js" defer crossorigin="anonymous" integrity="sha384/…"></script>Compute the correct SRI hashes (e.g. via
openssl dgst -sha384 -binary | openssl base64 -A) and replace each….
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/edit.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
🧠 Learnings (4)
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:06.803Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:376-379
Timestamp: 2025-09-24T18:52:06.803Z
Learning: In WLED firmware, all files reside in the root directory ("/"), so file tree refresh operations should always target the root path regardless of the parameter passed to refreshPath().
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T03:51:29.583Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.583Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:34.090Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.090Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.
Applied to files:
wled00/data/edit.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wled00/wled_server.cpp (2)
319-334: Consider blocking deletion of wsec.json to align with other protections.List/edit/download already hide/forbid wsec.json. Deleting it via DELETE is still allowed; confirm policy or block it.
Apply if you want to forbid deletion:
String path = request->getParam(FPSTR(s_path), true)->value(); if (!path.startsWith("/")) path = "/" + path; + // prevent deleting WiFi/security secrets + if (path.indexOf(FPSTR(s_wsec)) >= 0) { + request->send(403, FPSTR(CONTENT_TYPE_PLAIN), FPSTR(s_accessdenied)); + return; + } + if (!WLED_FS.remove(path)) { request->send(500, FPSTR(CONTENT_TYPE_PLAIN), F("Delete failed")); return; }
239-277: Build JSON with proper escaping.Manually concatenating names risks malformed JSON if a filename contains quotes or backslashes. Prefer AsyncJsonResponse (or escape names).
Example (ESP32 branch sketch):
AsyncJsonResponse* res = new AsyncJsonResponse(false); JsonArray arr = res->getRoot().to<JsonArray>(); // ... for each file: JsonObject o = arr.createNestedObject(); o["name"] = name; // safely escaped o["type"] = "file"; o["size"] = file.size(); res->setLength(); request->send(res);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/edit.htm(1 hunks)wled00/wled_server.cpp(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for all C++ source and header files
Files:
wled00/wled_server.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
🧠 Learnings (6)
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-25T03:51:29.583Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.583Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/wled_server.cppwled00/data/edit.htm
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:06.803Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:376-379
Timestamp: 2025-09-24T18:52:06.803Z
Learning: In WLED firmware, all files reside in the root directory ("/"), so file tree refresh operations should always target the root path regardless of the parameter passed to refreshPath().
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:34.090Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.090Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T05:52:09.019Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-222
Timestamp: 2025-09-25T05:52:09.019Z
Learning: In the WLED file editor (wled00/data/edit.htm), file renaming upon upload is not supported. The filename field is used for other purposes like creating new files or saving edited content, but uploaded files keep their original names.
Applied to files:
wled00/data/edit.htm
🧬 Code graph analysis (1)
wled00/wled_server.cpp (2)
tools/cdata.js (2)
path(19-19)output(29-29)wled00/data/common.js (1)
path(85-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (4)
wled00/wled_server.cpp (1)
770-772: Enable editor after PIN acceptance: LGTM.This addresses the earlier 401-after-PIN issue.
wled00/data/edit.htm (3)
211-224: Upload flow: using original filename only is consistent with design.Renaming on upload is intentionally unsupported; implementation matches that.
255-263: Image preview relies on server MIME; verify after server fix.With /edit?edit now serving correct image content types, preview should render reliably across browsers.
359-365: Always refreshing root is fine given flat FS.Matches WLED’s root-only file layout.
wled00/wled_server.cpp
Outdated
| if (request->hasParam(FPSTR(s_list))) { | ||
| String path = request->getParam(FPSTR(s_list))->value(); | ||
| if (path.isEmpty()) path = "/"; | ||
| String output = "["; |
There was a problem hiding this comment.
😬 Arduino String concatenation is still one of the worst ways to abuse your allocator ....
Instead use AsyncResponseStream which is an Arduino Print type that will build up a buffer in packet sized blocks:
AsyncResponseStream* response = request->beginResponseStream(FPSTR(CONTENT_TYPE_JSON));
response->write('{'); // character literal can be inlined
for_each(file) {
if (!first) response->write(',');
response->printf_P(PSTR({\"name\":\"%s\",\"type\":\"file\",\"size\":%d}"), file.name.c_str(), file.size());
}
response->write('}');
request->send(response);
(also you might want to send a please-don't-cache header, like serveSettingsJS(); also an example of AsyncResponseStream.)
There was a problem hiding this comment.
thanks, that is helpful. I looked at some examples but completely missed out on AsyncResponseStream. I also found there are other deficiencies in the code, should not have relied so heavily on AI to get it right :)
Rewrote the whole section but found some other things I want to update, the editor UI is a bit convoluted and can be simplified.
|
@coderabbitai please review latest commits |
|
@DedeHai I'll review the latest commits in your new file editor PR. This looks like a comprehensive replacement of the existing editor with modern functionality. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/edit.htm(1 hunks)wled00/wled_server.cpp(7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:06.803Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:376-379
Timestamp: 2025-09-24T18:52:06.803Z
Learning: In WLED firmware, all files reside in the root directory ("/"), so file tree refresh operations should always target the root path regardless of the parameter passed to refreshPath().
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T03:51:29.583Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.583Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:34.090Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.090Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T05:52:09.019Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-222
Timestamp: 2025-09-25T05:52:09.019Z
Learning: In the WLED file editor (wled00/data/edit.htm), file renaming upon upload is not supported. The filename field is used for other purposes like creating new files or saving edited content, but uploaded files keep their original names.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-20T09:00:08.942Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-20T09:00:08.942Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated headers wled00/html_*.h
Applied to files:
wled00/wled_server.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Fixed all issues hopefully. @willmmiles can you glance at the updated handler code? if you approve, this is ready to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
wled00/data/edit.htm (4)
279-279: Eliminate code duplication by using the tree parameter.An inline object with a
refreshPathmethod is created here, duplicating logic that already exists in thetreeobject passed todelFile. This violates DRY principles and makes the code harder to maintain.Apply this diff:
- deleteOption.onclick = function() { delFile(path, {refreshPath: function() { treeRoot.innerHTML=""; httpGet("/"); }}); menu.remove(); }; + deleteOption.onclick = function() { delFile(path, tree); menu.remove(); };Note: This requires capturing the
treeobject in thecreateLeaffunction scope. Update thecreateTreefunction to pass a reference:function createTree(element, editor){ var treeRoot=cE("div"); gId(element).appendChild(treeRoot); + var tree = { refreshPath: function(p) { treeRoot.innerHTML=""; httpGet("/"); } }; // ... (downlFile and delFile stay the same) function createLeaf(path,name){ // ... existing code ... - deleteOption.onclick = function() { delFile(path, {refreshPath: function() { treeRoot.innerHTML=""; httpGet("/"); }}); menu.remove(); }; + deleteOption.onclick = function() { delFile(path, tree); menu.remove(); }; // ... rest of createLeaf ... } // ... rest of createTree ... - this.refreshPath=function(p){ + tree.refreshPath=function(p){ treeRoot.innerHTML=""; httpGet("/"); }; httpGet("/"); - return this; + return tree; }
565-565: Consider passing tree reference explicitly instead of using global state.Storing the tree reference in
window.fileTreecreates a global dependency that makes the code harder to test and maintain. The editor already receives the tree indirectly through callbacks.Consider refactoring to pass the tree reference explicitly:
-function createEditor(element,file){ +function createEditor(element,file,tree){ if (!file) file=""; // ... existing code ... function httpPost(filename,data){ // ... existing code ... req.add("POST","/upload",fd,function(st,resp){ if (st!=200) alert("ERROR "+st+": "+resp); else { alert("File saved successfully!"); - if (window.fileTree) window.fileTree.refreshPath("/"); + if (tree) tree.refreshPath("/"); } }); } // ... rest of function ... } function onBodyLoad(){ var vars={}; window.location.href.replace(/[?&]+([^=&]+)=([^&]*)/gi,function(m,k,v){ vars[decodeURIComponent(k)]=decodeURIComponent(v); }); - var editor=createEditor("editor",vars.file); var tree=createTree("tree",editor); - window.fileTree = tree; + var editor=createEditor("editor",vars.file,tree); createTop("top",tree,editor); }Note: This requires restructuring the initialization order or using a forward reference pattern since
createTreeneeds the editor andcreateEditorwould need the tree.
148-158: Consider using createElement for DOM manipulation instead of innerHTML.While the current implementation is safe due to
encodeURIComponent, usingcreateElementis more idiomatic and avoids any theoretical string injection risks.Apply this diff:
function loadPreview(filename, editor) { var pathField = gId("filepath"); pathField.value = filename; if (/\.(png|jpg|jpeg|gif|bmp|webp)$/i.test(filename)) { gId("editor").style.display="none"; gId("preview").style.display="block"; - gId("preview").innerHTML = '<img src="/edit?func=edit&path=' + encodeURIComponent(filename) + '&_cb=' + Date.now() + '">'; + var img = cE("img"); + img.src = "/edit?func=edit&path=" + encodeURIComponent(filename) + "&_cb=" + Date.now(); + var preview = gId("preview"); + preview.innerHTML = ""; + preview.appendChild(img); } else { editor.loadText(filename); } }
333-363: Consider performance impact for very large LED maps.The
prettyLedmapfunction performs string padding for each number in the map, which could be slow for maps with thousands of LEDs. For typical use cases this is fine, but consider adding a size check for very large maps.Add an optional size threshold:
function prettyLedmap(json){ try { let obj = JSON.parse(json); if (!obj.map || !Array.isArray(obj.map)) return JSON.stringify(obj, null, 2); + + // For very large maps, skip pretty-printing to avoid performance issues + if (obj.map.length > 10000) { + return JSON.stringify(obj, null, 2); + } + let width = obj.width || obj.map.length; let maxLen = Math.max(...obj.map.map(n => String(n).length)); // ... rest of function ... } catch (e) { return json; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/edit.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/edit.htm
🧠 Learnings (5)
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:06.843Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:376-379
Timestamp: 2025-09-24T18:52:06.843Z
Learning: In WLED firmware, all files reside in the root directory ("/"), so file tree refresh operations should always target the root path regardless of the parameter passed to refreshPath().
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:34.117Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.117Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T03:51:29.606Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T05:52:09.044Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-222
Timestamp: 2025-09-25T05:52:09.044Z
Learning: In the WLED file editor (wled00/data/edit.htm), file renaming upon upload is not supported. The filename field is used for other purposes like creating new files or saving edited content, but uploaded files keep their original names.
Applied to files:
wled00/data/edit.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
🔇 Additional comments (4)
wled00/data/edit.htm (4)
420-422: LGTM: Robust ACE editor initialization with fallback.The dual initialization strategy (immediate timeout + window load event) ensures ACE loads even if the CDN is slow, while the guard clause in
initAceprevents duplicate initialization. This is a solid defensive pattern.
519-531: LGTM: Save-then-reload sequence is safe due to request queuing.The
httpGet(fn)call afterhttpPostis protected from race conditions by theQueuedRequestermechanism, which ensures requests execute sequentially. The reload will only happen after the save completes successfully.
449-463: LGTM: Clear visual feedback for JSON validation.The live JSON validation with border color feedback provides immediate user feedback without being intrusive. The filename-based detection ensures validation only runs for JSON files.
216-228: LGTM: File upload implementation is correct.The upload flow properly handles file selection, FormData construction, error handling, and post-upload cleanup. Using
file.namedirectly aligns with the design decision that file renaming on upload is not supported. Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wled00/data/edit.htm (1)
10-12: Add Subresource Integrity (SRI) hashes for CDN resources.Loading scripts from external CDNs without SRI attributes creates a security posture gap. If the CDN is compromised or serves malicious content, the editor could execute untrusted code.
Generate the SRI hashes and add them:
#!/bin/bash # Generate SRI hashes for ACE editor CDN resources for url in \ "https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js" \ "https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js" \ "https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js" do echo "URL: $url" curl -s "$url" | openssl dgst -sha512 -binary | openssl base64 -A | sed 's/^/sha512-/' echo -e "\n" doneThen apply this pattern to each script tag:
-<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js"></script> +<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js" + integrity="sha512-GENERATED_HASH" crossorigin="anonymous"></script>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/edit.htm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/edit.htm
🧠 Learnings (5)
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
PR: wled/WLED#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:06.843Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:376-379
Timestamp: 2025-09-24T18:52:06.843Z
Learning: In WLED firmware, all files reside in the root directory ("/"), so file tree refresh operations should always target the root path regardless of the parameter passed to refreshPath().
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:34.117Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.117Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T03:51:29.606Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T05:52:09.044Z
Learnt from: DedeHai
PR: wled/WLED#4956
File: wled00/data/edit.htm:213-222
Timestamp: 2025-09-25T05:52:09.044Z
Learning: In the WLED file editor (wled00/data/edit.htm), file renaming upon upload is not supported. The filename field is used for other purposes like creating new files or saving edited content, but uploaded files keep their original names.
Applied to files:
wled00/data/edit.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
🔇 Additional comments (14)
wled00/data/edit.htm (14)
14-114: LGTM! CSS styling is well-structured.The CSS uses tabs for indentation as per coding guidelines, defines clear component layouts, and customizes ACE editor colors to match WLED style.
116-154: LGTM! Request queuing implementation is solid.The queued request handler properly serializes requests, handles both GET and POST methods, supports FormData, and manages loader visibility. The implementation prevents race conditions and handles different parameter types correctly.
157-167: LGTM! Preview loading is correctly implemented.The function properly handles image vs. text file detection and uses
encodeURIComponentto safely embed filenames in URLs. The cache-busting timestamp prevents stale image displays.
169-171: LGTM! Tree refresh helper is straightforward.Simple wrapper that calls the global tree's refresh method.
173-241: LGTM! Top control bar implementation is well-structured.The upload/save/clear/back button handlers are correctly wired. File uploads use the original filename (as intended per WLED design), and success callbacks properly refresh the tree and load previews.
306-340: LGTM! File list rendering and refresh logic is correct.The tree properly filters sensitive files (wsec.json), constructs file entries, and refreshes to the root directory (as all files are in root per WLED design). The defense-in-depth approach to hiding wsec.json is appropriate.
Based on learnings
342-372: LGTM! Ledmap pretty-printer is well-implemented.The function correctly formats 2D LED maps with aligned columns while preserving other JSON properties. The fallback to standard formatting on errors is appropriate.
389-421: LGTM! ACE editor initialization is robust.The editor correctly falls back to textarea if ACE is unavailable, sets up JSON mode detection, and prevents duplicate event listeners. The deferred initialization pattern handles CDN loading timing.
446-459: LGTM! Live JSON validation provides good UX.The textarea validation provides immediate visual feedback with red borders for invalid JSON, helping users catch syntax errors early.
461-481: LGTM! Save function with JSON minification is appropriate.The function properly validates JSON before saving and minifies to reduce storage. Error handling with alerts provides clear feedback.
483-520: LGTM! File loading and save handler are correctly implemented.The load function properly handles JSON pretty-printing (including special ledmap formatting), falls back gracefully on parse errors, and updates editor mode. The save handler validates the filename and updates state appropriately.
521-539: LGTM! Public API methods are clean and functional.The returned API object provides clear methods for save, loadText, and clearEditor operations with appropriate state management and placeholder text.
542-551: LGTM! Initialization sequence is correct.The onBodyLoad function properly parses URL parameters and initializes components in the right order (editor → tree → top), allowing dependencies between components.
554-565: LGTM! HTML body structure is clean.The body structure matches the CSS layout with proper nesting and includes all required containers for the editor, tree, preview, and loader.
|
@willmmiles friendly reminder to check wled_server.cpp changes :) |
willmmiles
left a comment
willmmiles
left a comment
There was a problem hiding this comment.
Sorry it's taken me so long to get back to you on this!
…mory usage to edit.htm - memory usage per file and total now showing in editor - added getURL() for better proxy support
|
@willmmiles I made the suggested changes, tested and it all seems to be working as intended. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wled00/wled_server.cpp (1)
302-302: Optional: Remove unnecessary return statement.The empty return at the end of the handler is unreachable and can be removed for cleaner code.
Apply this diff:
request->send(200, FPSTR(CONTENT_TYPE_PLAIN), F("File deleted")); return; } - - return; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wled00/data/edit.htm(1 hunks)wled00/fcn_declare.h(0 hunks)wled00/set.cpp(0 hunks)wled00/util.cpp(0 hunks)wled00/wled.cpp(0 hunks)wled00/wled_server.cpp(7 hunks)
💤 Files with no reviewable changes (4)
- wled00/fcn_declare.h
- wled00/wled.cpp
- wled00/set.cpp
- wled00/util.cpp
🧰 Additional context used
📓 Path-based instructions (3)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/wled_server.cpp
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Files:
wled00/data/edit.htm
wled00/data/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**: When modifying web UI files, runnpm run buildto regenerate embedded headers before any firmware build
For web UI changes, edit files only under wled00/data (not firmware or generated files)
Files:
wled00/data/edit.htm
🧠 Learnings (20)
📓 Common learnings
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-25T03:51:29.606Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/wled_server.cppwled00/data/edit.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/wled_server.cppwled00/data/edit.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-05-26T16:09:34.325Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/wled_server.cppwled00/data/edit.htm
📚 Learning: 2025-08-29T19:51:15.511Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/open_weather_map_source.cpp:13-19
Timestamp: 2025-08-29T19:51:15.511Z
Learning: On ESP8266/ESP32 devices with limited heap, using HTTPS for API calls that return large JSON responses may not be feasible due to the combined memory requirements of TLS handshake and JSON parsing. HTTP may be necessary despite security concerns.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-09T23:21:00.549Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4919
File: usermods/usermod_v2_departstrip/readme.md:39-49
Timestamp: 2025-09-09T23:21:00.549Z
Learning: For the DepartStrip usermod on ESP8266/ESP32 devices, HTTP is preferred over HTTPS in documentation examples and templates due to heap memory constraints. The combined memory requirements of TLS handshake and JSON parsing for transit API responses can exceed available memory on these embedded devices.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/!(html_*)*.h : Use 2-space indentation for non-generated C++ header files (.h)
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/**/*.cpp : Use 2-space indentation for C++ source files (.cpp)
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/index.htm : Main web interface entry file is index.htm; ensure it remains present and functional
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:06.843Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:376-379
Timestamp: 2025-09-24T18:52:06.843Z
Learning: In WLED firmware, all files reside in the root directory ("/"), so file tree refresh operations should always target the root path regardless of the parameter passed to refreshPath().
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-24T18:52:34.117Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:295-303
Timestamp: 2025-09-24T18:52:34.117Z
Learning: WLED file system stores all files in the root directory only (flat structure), so file paths don't have subdirectories. When working with WLED file editors, using just the filename (name) rather than full path is correct since there are no nested directories.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-09-25T05:52:09.044Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-222
Timestamp: 2025-09-25T05:52:09.044Z
Learning: In the WLED file editor (wled00/data/edit.htm), file renaming upon upload is not supported. The filename field is used for other purposes like creating new files or saving edited content, but uploaded files keep their original names.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-10-11T10:49:13.498Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:295-295
Timestamp: 2025-10-11T10:49:13.498Z
Learning: In the WLED codebase (wled00/data/), the variable `d` is defined in common.js as `var d=document;` and serves as a shorthand alias for `document`. This is a valid convention used throughout WLED's JavaScript files, so references to `d.getElementById()`, `d.body`, `d.onclick`, etc. should not be flagged as undefined variable errors.
Applied to files:
wled00/data/edit.htm
📚 Learning: 2025-10-05T15:41:15.810Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4982
File: wled00/data/matrixtool/omggif.js:32-35
Timestamp: 2025-10-05T15:41:15.810Z
Learning: The file `wled00/data/matrixtool/omggif.js` is an external third-party library from https://github.com/deanm/omggif by Dean McNamee and should not be reviewed for code issues as it's maintained externally.
Applied to files:
wled00/data/edit.htm
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (13)
wled00/wled_server.cpp (6)
20-37: LGTM - Efficient flash memory usage.The new PROGMEM strings for editor routing and caching headers follow WLED conventions and reduce flash memory usage.
236-261: LGTM - Efficient streaming approach.The file list handler correctly uses
AsyncResponseStreamto avoid String concatenation overhead and properly filterswsec.json.
263-300: LGTM - Comprehensive security checks.Path validation, file existence checks, and
wsec.jsonblocking are properly implemented. Content-type auto-detection will work correctly for image previews.
209-209: LGTM - Concise response message.The updated response message is clearer and more concise.
469-469: LGTM - Simplified initialization.The handler initialization is correctly simplified, with PIN protection implemented inside the handler itself.
82-84: LGTM - Consistent use of FPSTR constants.Using PROGMEM string constants for cache control headers reduces memory usage.
Also applies to: 647-648
wled00/data/edit.htm (7)
116-154: LGTM - Clean request queuing implementation.The
QueuedRequesterclass properly manages concurrent requests with loader feedback and correctly handles both FormData and URL-encoded parameters.
157-171: LGTM - Proper image handling with cache-busting.Image detection by extension and cache-busting with timestamps ensure fresh previews.
173-242: LGTM - Well-structured upload flow.The upload handling correctly uses the original filename and provides user feedback. The 31-character filename limit is reasonable for embedded systems.
244-359: LGTM - Robust file tree with proper cleanup.The file tree implementation includes proper event cleanup for context menus and correctly filters sensitive files.
361-391: LGTM - Clever ledmap formatting with proper fallback.The pretty-printing logic for ledmap files handles edge cases gracefully and falls back to original content on errors.
393-559: LGTM - Comprehensive editor with graceful fallback.The editor implementation properly handles ACE initialization failures, provides live JSON validation, and includes special formatting for ledmap files. The save/load flow with JSON minification is well-designed.
561-583: LGTM - Clean initialization flow.The initialization sequence and HTML structure are properly coordinated with the CSS layout.
| <script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js"></script> | ||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js"></script> | ||
| <script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js"></script> |
There was a problem hiding this comment.
🧩 Analysis chain
SRI hashes remain missing for CDN resources.
This security posture gap was previously flagged. While ACE editor loading has a fallback to textarea, adding SRI hashes would protect against CDN compromise.
Generate SRI hashes:
🏁 Script executed:
#!/bin/bash
# Generate SRI hashes for ACE editor CDN resources
for url in \
"https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js" \
"https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js" \
"https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js"
do
echo "URL: $url"
curl -s "$url" | openssl dgst -sha512 -binary | openssl base64 -A | sed 's/^/sha512-/'
echo -e "\n"
doneLength of output: 495
🏁 Script executed:
#!/bin/bash
# 1. Verify current state of edit.htm lines 10-12
echo "=== Current state of edit.htm lines 10-12 ==="
sed -n '10,12p' wled00/data/edit.htm
echo -e "\n=== Attempt to generate SRI hashes using Python ==="
python3 << 'EOF'
import hashlib
import base64
import urllib.request
import sys
urls = [
"https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js",
"https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js",
"https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js"
]
for url in urls:
try:
print(f"URL: {url}")
response = urllib.request.urlopen(url, timeout=5)
content = response.read()
sha512_hash = hashlib.sha512(content).digest()
sri_hash = "sha512-" + base64.b64encode(sha512_hash).decode('ascii')
print(f"SRI Hash: {sri_hash}")
print()
except Exception as e:
print(f"Error: {e}\n")
EOFLength of output: 991
🌐 Web query:
ACE editor 1.23.4 SRI hash integrity cdnjs
💡 Result:
Which specific file do you need the SRI hash for (e.g. ace.js, ace.min.js, or a specific extension/mode) from cdnjs v1.23.4?
🏁 Script executed:
#!/bin/bash
# Check if other script tags in wled00/data use SRI hashes
echo "=== Checking for SRI hash usage in wled00/data files ==="
grep -r "integrity=" wled00/data/ --include="*.htm" --include="*.html" | head -20
echo -e "\n=== Checking all script tags in edit.htm ==="
grep -n "<script" wled00/data/edit.htmLength of output: 432
🌐 Web query:
SRI integrity subresource integrity CDN security critical
💡 Result:
Short summary — what SRI is and why it's critical for CDN security
- Subresource Integrity (SRI) lets you embed a cryptographic hash in a <script> or tag so the browser verifies a fetched third‑party resource matches the expected contents; if the hash differs the browser blocks execution. [1][2]
- This prevents supply‑chain attacks or CDN compromises from executing tampered JavaScript/CSS on your pages — i.e., it turns a silent compromise into a load/availability failure instead of arbitrary code execution. [1][3]
Key limitations and pitfalls to know
- CORS requirement: for cross‑origin resources the server must permit CORS (Access‑Control‑Allow‑Origin) for SRI checks to apply. Without appropriate CORS headers the resource may not be verifiable. [1][2]
- Fragile with changing builds: any update to the CDN asset (new minification, patch, different build) changes the hash — you must regenerate and deploy new integrity attributes. This makes SRI unsuitable for assets that change frequently unless you automate hash updates. [2][5]
- Not a substitute for all controls: SRI only protects the exact hashed asset. It doesn’t control what different third‑party endpoints load at runtime (e.g., dynamic imports, other scripts), and older browsers ignore the attribute. Use SRI alongside CSP, strong HTTPS, subresource policy headers, and monitoring. [1][2][3]
- Inline scripts/styles: SRI protects external files only; inline code must be controlled with CSP nonces/hashes or Integrity‑Policy headers. [1]
Practical best practices
- Always add integrity (sha256/384/512) plus crossorigin="anonymous" when including stable CDN libraries. Automate hash generation in your build pipeline. [2][5]
- Use sha384 or sha512 (stronger than sha256) per recommendations and browser support guidance. [1][2]
- Combine SRI with Content Security Policy and Integrity‑Policy/Report‑Only headers for broader enforcement and reporting. [1][2]
- Prefer pinned, versioned CDN URLs (immutable builds) to reduce churn and re‑pinning frequency. [2][5]
References
- MDN — Subresource Integrity (overview & implementation). [1][2]
- SecurityScorecard / vulnerability guidance on unsafe SRI usage. [3]
- Wikipedia — Subresource Integrity summary. [4]
Add SRI integrity hashes to external ACE editor CDN scripts.
The three external script resources on lines 10-12 lack Subresource Integrity hashes, which are a critical security control to prevent supply-chain attacks and CDN compromises. Subresource Integrity protects against CDN compromise by enabling browsers to verify fetched third‑party resources match expected contents; if the hash differs the browser blocks execution.
Add integrity="sha384-[HASH]" and crossorigin="anonymous" to each script tag:
<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/ace.min.js" integrity="sha384-[HASH]" crossorigin="anonymous"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/mode-json.min.js" integrity="sha384-[HASH]" crossorigin="anonymous"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/ace/1.23.4/theme-monokai.min.js" integrity="sha384-[HASH]" crossorigin="anonymous"></script>Generate SHA-384 hashes for each URL using online SRI generators (e.g., srihash.org) or locally with: curl -s [URL] | openssl dgst -sha384 -binary | openssl base64 -A.
🤖 Prompt for AI Agents
In wled00/data/edit.htm around lines 10 to 12 the three external ACE editor CDN
script tags lack Subresource Integrity (SRI) and crossorigin attributes;
generate SHA-384 base64 hashes for each exact URL (e.g., curl -s [URL] | openssl
dgst -sha384 -binary | openssl base64 -A or use an SRI generator) and add
integrity="sha384-[GENERATED_HASH]" and crossorigin="anonymous" to each
corresponding <script> tag, replacing [GENERATED_HASH] with the computed value
so the browser can verify the resource.
There was a problem hiding this comment.
@willmmiles what do you think of this rabbit comment? worth spending the extra code?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
While in general that seems like a best practice, back of the envelope we'd be looking at ~250 bytes of code space for hashes. I'm not sure it's worth the tradeoff for us. (Famous last words, I know!) Could try it and see what the difference actually is.
There was a problem hiding this comment.
It's just locking the dependency, let's save the space
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/wled_server.cpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/wled_server.cpp
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-25T03:51:29.606Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-05-26T16:09:34.325Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T19:51:15.511Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/open_weather_map_source.cpp:13-19
Timestamp: 2025-08-29T19:51:15.511Z
Learning: On ESP8266/ESP32 devices with limited heap, using HTTPS for API calls that return large JSON responses may not be feasible due to the combined memory requirements of TLS handshake and JSON parsing. HTTP may be necessary despite security concerns.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-09T23:21:00.549Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4919
File: usermods/usermod_v2_departstrip/readme.md:39-49
Timestamp: 2025-09-09T23:21:00.549Z
Learning: For the DepartStrip usermod on ESP8266/ESP32 devices, HTTP is preferred over HTTPS in documentation examples and templates due to heap memory constraints. The combined memory requirements of TLS handshake and JSON parsing for transit API responses can exceed available memory on these embedded devices.
Applied to files:
wled00/wled_server.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (5)
wled00/wled_server.cpp (5)
20-36: LGTM!The new include and PROGMEM constants follow WLED's established patterns and will reduce flash memory usage through string deduplication.
220-235: LGTM! Clean initialization with proper PIN gating.The simplified signature (removed
bool enableparameter) and internal PIN check align with the previous review feedback. The handler is now created once with access control enforced inside, avoiding unnecessary heap churn.
237-262: Excellent use of AsyncResponseStream!This implementation addresses the previous feedback to avoid String concatenation and properly uses
AsyncResponseStreamfor efficient streaming JSON generation. The wsec.json filtering and proper file cleanup are also well done.
264-293: LGTM! Path validation and security checks are solid.The path normalization, existence checks, and wsec.json blocking provide good security boundaries. Using
request->send(WLED_FS, path)without explicit content-type (line 286) correctly allows the library to deduce MIME types, fixing the image preview concern raised in earlier reviews.
470-470: LGTM! Consistent initialization and cache header updates.The single
createEditHandler()call during initialization (line 470) with the clarifying comment confirms the simplified access control model. The cache header updates (lines 648-649) maintain consistency with the FPSTR optimization pattern used throughout the file.Also applies to: 648-649
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/wled_server.cpp(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/wled_server.cpp
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-14T18:43:59.338Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4932
File: wled00/data/cpal/cpal.htm:14-14
Timestamp: 2025-09-14T18:43:59.338Z
Learning: In WLED, static web pages like cpal.htm are served directly at the root URL level (e.g., "/cpal.htm") via handleStaticContent() from embedded page data, regardless of their physical source folder location in the repository. Relative script references like "common.js" resolve correctly from the served URL path, not the source file path.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-25T03:51:29.606Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4956
File: wled00/data/edit.htm:213-238
Timestamp: 2025-09-25T03:51:29.606Z
Learning: For WLED file editor: Path handling can be simplified since the system operates in root directory context, complex path traversal protection is not needed.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-05T15:24:05.545Z
Learnt from: CR
Repo: wled/WLED PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T15:24:05.545Z
Learning: Applies to wled00/data/index.htm : Main web interface entry file is index.htm; ensure it remains present and functional
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-05-26T16:09:34.325Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4700
File: wled00/wled_server.cpp:409-414
Timestamp: 2025-05-26T16:09:34.325Z
Learning: In wled00/wled_server.cpp, the OTA update access control logic intentionally allows OTA updates from different subnets when otaSameSubnet is true AND a PIN is set. This was a conscious design decision by blazoncek to provide flexibility for remote OTA updates with PIN protection, though they acknowledged it may not be the optimal security model.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR #4798, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T19:51:15.511Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/open_weather_map_source.cpp:13-19
Timestamp: 2025-08-29T19:51:15.511Z
Learning: On ESP8266/ESP32 devices with limited heap, using HTTPS for API calls that return large JSON responses may not be feasible due to the combined memory requirements of TLS handshake and JSON parsing. HTTP may be necessary despite security concerns.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
Repo: wled/WLED PR: 4883
File: usermods/usermod_v2_skystrip/rest_json_client.h:6-14
Timestamp: 2025-08-29T00:26:15.808Z
Learning: WLED uses a vendored ArduinoJson library (version 6) located at "src/dependencies/json/ArduinoJson-v6.h" which is included through wled.h. Usermods should not directly include ArduinoJson headers but instead rely on wled.h for ArduinoJson symbols. The standard pattern is to include wled.h and use JsonObject, JsonArray, DynamicJsonDocument, etc. without additional includes.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-18T03:17:30.107Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4939
File: wled00/FX_fcn.cpp:1183-1194
Timestamp: 2025-09-18T03:17:30.107Z
Learning: In WLED bus management code, dead code branches in error handling (like unreachable else breaks) are intentionally preserved to accommodate potential future API changes, even when currently unreachable.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-10-10T18:34:06.550Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4997
File: wled00/e131.cpp:33-44
Timestamp: 2025-10-10T18:34:06.550Z
Learning: In WLED's DDP packet handling (ws.cpp and e131.cpp), only prevent out-of-bounds memory access rather than enforcing DDP spec compliance. Don't check the 1440-byte spec limit—accept out-of-spec packets assuming correct encoding. The bounds check `maxDataIndex = c + numLeds * ddpChannelsPerLed; if (maxDataIndex > dataLen) reject` is sufficient and already covers the timecode flag case (when c=4) without needing separate validation.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-28T08:09:20.630Z
Learnt from: mval-sg
Repo: wled/WLED PR: 4876
File: wled00/xml.cpp:0-0
Timestamp: 2025-08-28T08:09:20.630Z
Learning: The WLED codebase has opportunities for refactoring hardcoded array bounds (like the "15" used for DMX channels) to use sizeof(array)/sizeof(array[0]) for more maintainable code, but such changes should be done consistently across the entire codebase in a dedicated refactoring effort.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-08-21T00:00:17.196Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-09-28T09:53:42.670Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4939
File: wled00/data/index.js:2406-2413
Timestamp: 2025-09-28T09:53:42.670Z
Learning: In WLED, code paths that appear unreachable may actually be safety fallbacks for low-heap conditions. These fallbacks are intentional defensive programming measures to ensure UI functionality is preserved when system resources are constrained.
Applied to files:
wled00/wled_server.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (5)
wled00/wled_server.cpp (5)
20-20: LGTM - Flash string constants for new editor.The new PROGMEM constants follow WLED conventions and support the refactored file editor functionality.
Also applies to: 30-36
82-82: LGTM - Cache header string optimization.Refactoring cache control headers to use FPSTR constants reduces flash usage and follows WLED conventions.
Also applies to: 84-84, 649-650
220-235: LGTM - Simplified handler registration with PIN guard.The refactored approach keeps the handler always registered and checks PIN at the entry point, eliminating heap churn from dynamic handler recreation. Editor page serving is correct with proper early return.
Based on learnings
264-304: LGTM - Path validation and file operations are correct.The implementation properly:
- Validates path parameter presence and file existence
- Blocks
wsec.jsonaccess for security- Uses auto content-type detection per review feedback
- Handles all functions with appropriate success/error responses
- Returns error for unrecognized functions
Based on learnings
471-471: LGTM - Handler initialization follows refactored approach.The handler is initialized once with PIN protection handled internally, eliminating the previous dynamic enable/disable pattern that caused heap fragmentation.
Based on learnings
3 participants
I made a new file editor with the help of our AI friends (I do suck at JS and HTML) so someone with more knowledge in web creation please check for potential issues.
This replaces the default SPIFFS-editor from the Async Webserver library.
Features:
removed restrictions to the editor in offline mode and mobile.
tested and working in chrome (PC and mobile), firefox and safari. Also checked ESP32 and ESP8266.
Bonus: saves 2k of flash
JSON file editing:
JSON error:
JSON error on saving:
Ledmap display:
image preview:
Edit: UI buttons updated:
Summary by CodeRabbit
New Features
Improvements
Behavior Changes