new file editor #4956
new file editor #4956
Conversation
WalkthroughAdds a rebuilt web editor page ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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. |
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