New custom palettes editor #5010
Conversation
separating iro.js adds about 400bytes to the bin file, this is due to reduced gzip compression when using it as an external file (no dictionary reuse in index.x)
link properly minifies, @import is skipped completely.
many changes to cpal UI, fixed bugs, added buttons (WIP), added cached loading
- load iro.js sequentially - no parallel requests in cpal.htm - update UI buttons - fix showing sequential loading of palettes (using opacity) - better UX for mobile (larger markers, larger editor) - various fixes
WalkthroughAdds iro.js as a served web asset and build output, replaces the palette editor UI with an iro-based deferred color picker, changes palette discovery to tolerate gaps, and switches palette removal to index-based deletion across frontend and backend. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Multiple heterogeneous changes across build tooling, server asset serving, frontend UI rewrite, and palette file handling require focused, cross-area review. Possibly related PRs
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 (7)
wled00/data/cpal/cpal.htm (3)
138-143: Freeze segments during preview to avoid effects overriding the gradientWhen enabling preview, freeze selected segments; unfreeze on disable/unload.
- gId('chkPreview').addEventListener('change', (e) => { - prvEn = e.target.checked; - if (prvEn) applyLED(); - else requestJson({seg:{frz:false}}); - }); + gId('chkPreview').addEventListener('change', async (e) => { + prvEn = e.target.checked; + if (prvEn) { + await requestJson({seg:{frz:true}}); + await applyLED(); + } else { + await requestJson({seg:{frz:false}}); + } + });
85-106: Cap and back off loader retries to avoid tight retry loops on low heap or 404Current 100ms infinite retries can spam the device. Use capped exponential backoff per resource.
+ let _cssRetry=0, _iroRetry=0, _comRetry=0; @@ - l1.onerror = () => setTimeout(() => document.head.appendChild(l1), 100); + l1.onerror = () => { + const d = Math.min(100 * (2 ** (_cssRetry++)), 4000); + if (_cssRetry <= 6) setTimeout(() => document.head.appendChild(l1), d); + }; @@ - l2.onerror = () => setTimeout(() => document.head.appendChild(l2), 100); + l2.onerror = () => { + const d = Math.min(100 * (2 ** (_iroRetry++)), 4000); + if (_iroRetry <= 6) setTimeout(() => document.head.appendChild(l2), d); + }; @@ - l3.onerror = () => setTimeout(() => document.head.appendChild(l3), 100); + l3.onerror = () => { + const d = Math.min(100 * (2 ** (_comRetry++)), 4000); + if (_comRetry <= 6) setTimeout(() => document.head.appendChild(l3), d); + };
76-81: Remove debug fetch overrideOverriding window.fetch is noisy and can surprise consumers. Recommend removing or gating behind a debug flag.
-const origFetch = window.fetch; -window.fetch = function(...args) { - console.log('FETCH:', args[0]); - return origFetch.apply(this, args); -}; +// Debug-only: consider enabling request logging behind a query flag if needed.usermods/audioreactive/audio_reactive.cpp (1)
1735-1739: rmcpal gating should not rely on customPalettes.size() (gaps now allowed).With gap-based discovery, rmcpal can target indices beyond loaded count. Gate on rmcpal presence (and local palettes>0), not vector size.
Apply:
- if (root.containsKey(F("rmcpal")) && root[F("rmcpal")].as<byte>() < customPalettes.size()) { + if (palettes > 0 && root.containsKey(F("rmcpal"))) { // handle removal of custom palettes from JSON call so we don't break things removeAudioPalettes(); }wled00/colors.cpp (1)
252-254: Right idea to reuse a single JsonDocument; consider sizing.1536 bytes may be higher than needed for current formats. If RAM constrained, measure and drop to the smallest safe capacity (e.g., 512–1024) or add a static_assert/comment tying capacity to max entries.
wled00/data/index.js (1)
47-64: Make iro.js loader more robust.Avoid tight retry loops and duplicate handlers; add backoff, cap retries, and mark listeners once.
Apply:
+(function(){ + let _iroRetry = 0; + function loadIro() { + const l = d.createElement('script'); + l.src = 'iro.js'; + l.async = true; l.defer = true; + l.onload = () => { + _iroRetry = 0; + cpick = new iro.ColorPicker("#picker", { width: 260, wheelLightness: false, wheelAngle: 270, wheelDirection: "clockwise", layout: [{component: iro.ui.Wheel, options: {}}] }); + (d.readyState === 'complete') ? onLoad() : window.addEventListener('load', onLoad, { once: true }); + }; + l.onerror = () => { + const delay = Math.min(1000 * Math.pow(2, _iroRetry++), 10000); + setTimeout(loadIro, delay); + }; + document.head.appendChild(l); + } + loadIro(); +})(); - -(function loadIro() { - const l = d.createElement('script'); - l.src = 'iro.js'; - l.onload = () => { - cpick = new iro.ColorPicker("#picker", { width: 260, wheelLightness: false, wheelAngle: 270, wheelDirection: "clockwise", layout: [{component: iro.ui.Wheel, options: {}}] }); - d.readyState === 'complete' ? onLoad() : window.addEventListener('load', onLoad); - }; - l.onerror = () => setTimeout(loadIro, 100); - document.head.appendChild(l); -})();wled00/data/index.htm (1)
363-371: Remember to rebuild embedded assets for web changes.After editing files under wled00/data, run “npm run build” to regenerate embedded headers (e.g., js_iro.h) before firmware build. As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignore(1 hunks)tools/cdata.js(1 hunks)usermods/audioreactive/audio_reactive.cpp(1 hunks)wled00/colors.cpp(2 hunks)wled00/const.h(1 hunks)wled00/data/cpal/cpal.htm(1 hunks)wled00/data/index.htm(3 hunks)wled00/data/index.js(3 hunks)wled00/json.cpp(1 hunks)wled00/wled_server.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/colors.cppwled00/wled_server.cppwled00/json.cpp
tools/cdata.js
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use tools/cdata.js to convert web assets to embedded C++ headers; do not reimplement this logic elsewhere
Files:
tools/cdata.js
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/const.h
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/index.jswled00/data/index.htmwled00/data/cpal/cpal.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/index.jswled00/data/index.htmwled00/data/cpal/cpal.htm
wled00/data/index.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Main web interface entry file is index.htm; ensure it remains present and functional
Files:
wled00/data/index.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/html_*.h : DO NOT edit generated embedded web header files (wled00/html_*.h)
Applied to files:
.gitignorewled00/wled_server.cpp
📚 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/** : For web UI changes, edit files only under wled00/data (not firmware or generated files)
Applied to files:
.gitignore
📚 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/html_*.h : Always commit generated html_*.h files along with related source changes
Applied to files:
.gitignore
📚 Learning: 2025-08-29T00:26:15.808Z
Learnt from: ksedgwic
PR: wled/WLED#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:
.gitignore
📚 Learning: 2025-04-26T19:19:07.600Z
Learnt from: blazoncek
PR: wled/WLED#4658
File: wled00/const.h:140-141
Timestamp: 2025-04-26T19:19:07.600Z
Learning: In WLED, the WLED_MAX_PANELS macro is intentionally defined as a fixed constant value (18) with no redefinition mechanism, making it "unoverridable" - there's no need for a static assertion to check its maximum value.
Applied to files:
wled00/const.h
🧬 Code graph analysis (1)
wled00/json.cpp (1)
wled00/colors.cpp (2)
loadCustomPalettes(248-296)loadCustomPalettes(248-248)
🔇 Additional comments (7)
.gitignore (1)
28-28: LGTM: ignore generated JS asset headersThe new /wled00/js_.h ignore matches the generated js_iro.h asset headers. Keep committing html_.h alongside UI source edits. As per coding guidelines.
wled00/wled_server.cpp (1)
13-13: iro.js static serving integration looks correctIncludes js_iro.h and serves /iro.js via handleStaticContent with FS override and ETag, consistent with /common.js. No issues spotted.
Also applies to: 31-31, 271-273
wled00/const.h (1)
18-18: LGTM: explicit scan-gap cap for custom palette discoveryThe guard is clear and documented. Ensure colors.cpp uses this define as intended.
usermods/audioreactive/audio_reactive.cpp (1)
1742-1746: Re-add condition looks good.Using WLED_MAX_CUSTOM_PALETTES avoids hardcoding. LGTM.
wled00/colors.cpp (1)
292-294: Gap threshold semantics.If “max consecutive missing files” is the intent, consider breaking on >= WLED_MAX_CUSTOM_PALETTE_GAP to match the name precisely. Verify desired behavior.
wled00/data/index.js (1)
1694-1703: Palette editor toggle hook is correct.Showing #editPal only when palette UI is active matches UX. LGTM.
wled00/data/index.htm (1)
132-133: Single “Custom palette editor” entry replaces add/remove — good consolidation.UI wiring aligns with index.js (#editPal). LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/data/cpal/cpal.htm (1)
648-648: Consider extracting magic numbers to named constants.The HTTP queue limit (
5), throttle delay (120ms), and preview delays (50/500ms) are currently inline magic numbers. While the code works correctly, extracting these to named constants would improve maintainability.For example:
+const HTTP_QUEUE_LIMIT = 5; +const HTTP_THROTTLE_DELAY = 120; +const PREVIEW_DELAY_WS = 50; +const PREVIEW_DELAY_HTTP = 500; + // ... later in code ... -if (_httpQueue.length >= 5) { +if (_httpQueue.length >= HTTP_QUEUE_LIMIT) { return Promise.resolve(-1); } // ... and ... -await new Promise(r => setTimeout(r, 120)); +await new Promise(r => setTimeout(r, HTTP_THROTTLE_DELAY));Also applies to: 666-666, 288-288
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
usermods/audioreactive/audio_reactive.cpp(1 hunks)wled00/data/cpal/cpal.htm(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- usermods/audioreactive/audio_reactive.cpp
🧰 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/cpal/cpal.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/cpal/cpal.htm
🔇 Additional comments (1)
wled00/data/cpal/cpal.htm (1)
195-196: No critical issues found—maxCol=50 value is well-documented and justified.The TODO comment reflects healthy caution, but verification confirms the limit is sound: line 73 documents that ESP8266 is limited to ~50 colors per chunk due to 500-byte websocket constraints, which aligns with the constraint enforced in
index.js(line 1737). The chunking logic inapplyLED()(line 695) correctly uses this value. While end-to-end testing on real hardware would be ideal, the implementation is justified by documented constraints and requires no changes.
1 participant
I made a new and improved custom palette editor.
Features:
All these features come at a cost of 2k of flash
Also fixed a bug in AR usermod regarding new "unlimited" custom palettes.
New added categories with 800+ palettes to choose from:
Editor and empty slot are "sticky":
On touch screens editor has larger handlers and larger editor gradient:
Summary by CodeRabbit
New Features
UI/UX Improvements
Behavior Changes