add json validation to file inputs in UI and minify before upload #5248
Conversation
… sequential loading in edit.htm
WalkthroughAdds JSON validation/minification to the upload flow, changes uploadFile signature to accept a callback, and refactors the editor page to load scripts/styles dynamically, use case-insensitive JSON detection, and call the new upload utility for saves (initialization hook renamed). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)wled00/data/**/*.{htm,html,css,js}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
wled00/data/**📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-11-27T13:54:01.277ZApplied to files:
📚 Learning: 2025-09-25T05:52:09.044ZApplied to files:
🧬 Code graph analysis (1)wled00/data/common.js (2)
⏰ 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)
🔇 Additional comments (4)
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 |
DedeHai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wled00/data/common.js (1)
142-151: Inconsistent indentation: use tabs instead of spaces.Lines 142, 144–150 use 2-space indentation, while the rest of the file uses tabs. As per coding guidelines, web files under
wled00/datashould use tabs for indentation.🔎 Proposed fix
async function uploadFile(fileObj, name, callback) { let file = fileObj.files?.[0]; // get first file, "?"" = optional chaining in case no file is selected - if (!file) { callback?.(false); return; } + if (!file) { callback?.(false); return; } if (/\.json$/i.test(name)) { // same as name.toLowerCase().endsWith('.json') - try { - const minified = JSON.stringify(JSON.parse(await file.text())); // validate and minify JSON - file = new Blob([minified], { type: file.type || "application/json" }); - } catch (err) { - if (!confirm("JSON invalid. Continue?")) { callback?.(false); return; } - // proceed with original file if invalid but user confirms - } - } + try { + const minified = JSON.stringify(JSON.parse(await file.text())); // validate and minify JSON + file = new Blob([minified], { type: file.type || "application/json" }); + } catch (err) { + if (!confirm("JSON invalid. Continue?")) { callback?.(false); return; } + // proceed with original file if invalid but user confirms + } + }wled00/data/edit.htm (2)
487-503: Double JSON validation occurs for.jsonfiles.The
saveFilefunction validates and minifies JSON (lines 489-496), thenuploadFileincommon.jswill parse and minify it again. This is harmless but redundant. Consider adding a flag to skip re-validation inuploadFilewhen the caller has already validated, or accept this minor overhead for simpler code.
540-540: Missing semicolon.Minor style inconsistency—this line lacks a trailing semicolon while other statements in the file use them.
🔎 Proposed fix
- saveFile(fn, getContent()) + saveFile(fn, getContent());
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/common.jswled00/data/edit.htm
🧰 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.htmwled00/data/common.js
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.htmwled00/data/common.js
🧠 Learnings (3)
📚 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/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/**/*.{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-11-27T13:54:01.277Z
Learnt from: DedeHai
Repo: wled/WLED PR: 5116
File: wled00/data/index.js:3474-3484
Timestamp: 2025-11-27T13:54:01.277Z
Learning: In WLED, the server-side upload handler automatically normalizes file paths by prepending a leading slash if missing (`if (finalname.charAt(0) != '/') { finalname = '/' + finalname; }`). This means client-side code uploading files (e.g., via `/upload` endpoint) can use paths with or without leading slashes—both will resolve to the same location.
Applied to files:
wled00/data/common.js
⏰ 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). (10)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev)
- 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 (esp32s3dev_8MB_opi)
🔇 Additional comments (4)
wled00/data/common.js (1)
140-160: LGTM on the overall JSON validation and callback pattern.The async file handling, JSON validation/minification logic, and callback signaling are implemented correctly. The function properly handles both successful uploads and error cases.
wled00/data/edit.htm (3)
114-122: LGTM on the dynamic resource loading approach.The IIFE with retry mechanism ensures
common.jsloads reliably before initializing the page. ChainingloadResourcesfor CSS follows the sequential loading pattern used elsewhere.
421-421: LGTM on case-insensitive JSON detection.The use of
/\.json$/iregex consistently across all JSON checks (lines 421, 477, 489, 511, 513) ensures robust handling of files regardless of extension casing.
563-582: LGTM on the initialization refactor.Renaming to
S()and triggering it vialoadResourcescallback ensures all dependencies are loaded before initialization. Removingonloadfrom<body>is the correct approach with this pattern.
|
I tested this with a valid and an invalid json file, works as intended. Also can confirm that pretty-printed json files are minified after upload i.e. when downloaded from the FS. |
1 participant
this prevents invalid files even making it to the file system.
also updated edit.htm to make use of that so even files uploaded through edit.htm benefit from this new functionality. While I was at it I also added sequential file loading in edit.htm like all other htm files already do.
Fixes #5246
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.