Use sequential loading and requests for all UI resources #5013
Conversation
- load common.js and style.css sequentially for all config pages - restrict all requrests in index.js to single connection
WalkthroughAdds a sequential JS/CSS resource loader and replaces static common.js/style.css includes with a retrying dynamic loader across many pages; refactors initialization/data loading in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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: 2
🧹 Nitpick comments (6)
wled00/data/common.js (1)
54-82: Bound retries + backoff; avoid perma-hidden UI and tight retry loopCurrent logic retries indefinitely every 100ms on load error, which can keep the UI hidden forever if a resource is missing or path is wrong. Add a bounded retry with backoff and a safe skip to the next resource, plus a once-only load handler and fix a small comment typo.
Apply this diff:
-function loadResources(files, init) { - let i = 0; +function loadResources(files, init, opts) { + let i = 0; + const retries = opts?.retries ?? 5; + const baseWait = opts?.delay ?? 100; // ms + const backoff = opts?.backoff ?? 1.5; // multiplier + const attempts = Object.create(null); const loadNext = () => { if (i >= files.length) { if (init) { - d.documentElement.style.visibility = 'visible'; // make page visible after all files are laoded if it was hidden (prevent ugly display) - d.readyState === 'complete' ? init() : window.addEventListener('load', init); + d.documentElement.style.visibility = 'visible'; // make page visible after all files are loaded if it was hidden (prevent ugly display) + if (d.readyState === 'complete') init(); + else window.addEventListener('load', init, { once: true }); } return; } const file = files[i++]; const isCSS = file.endsWith('.css'); const el = d.createElement(isCSS ? 'link' : 'script'); if (isCSS) { el.rel = 'stylesheet'; el.href = file; } else { el.src = file; } el.onload = () => { setTimeout(loadNext, 0);}; - el.onerror = () => { - i--; // load this file again - setTimeout(loadNext, 100); - }; + el.onerror = () => { + const n = (attempts[file] || 0) + 1; + attempts[file] = n; + if (n >= retries) { + console.warn(`loadResources: giving up on ${file} after ${n} attempts`); + // skip this file and continue with the rest + setTimeout(loadNext, 0); + return; + } + // retry same file with capped exponential backoff + const wait = Math.min(5000, Math.round(baseWait * Math.pow(backoff, n - 1))); + i--; // reattempt current file + setTimeout(loadNext, wait); + }; d.head.appendChild(el); }; loadNext(); }Optionally, let callers pass
{retries, delay, backoff}to tune behavior per page.wled00/data/settings_ui.htm (1)
11-18: Add bounded retries and minimal fallback for common.js loaderPrevent an infinite retry loop and blank page if common.js can’t be fetched. Add a small counter, backoff, and a user-visible fallback.
- (function loadFiles() { + (function loadFiles() { + let tries = 0; const l = document.createElement('script'); l.src = 'common.js'; - l.onload = () => loadResources(['style.css'], S); // load style.css then call S() - l.onerror = () => setTimeout(loadFiles, 100); + l.onload = () => loadResources(['style.css'], S); // load style.css then call S() + l.onerror = () => { + if (++tries >= 5) { + console.warn('Failed to load common.js after 5 attempts'); + d.documentElement.style.visibility = 'visible'; + alert('Failed to load common.js. Please reload the page.'); + return; + } + setTimeout(loadFiles, Math.min(5000, 100 * tries)); // simple backoff + }; document.head.appendChild(l); })();wled00/data/settings_time.htm (1)
11-18: Harden common.js retry loopSame as other settings pages: add bounded retries with backoff and a visible fallback to avoid an endlessly hidden page if common.js fails.
- (function loadFiles() { + (function loadFiles() { + let tries = 0; const l = document.createElement('script'); l.src = 'common.js'; l.onload = () => loadResources(['style.css'], S); // load style.css then call S() - l.onerror = () => setTimeout(loadFiles, 100); + l.onerror = () => { + if (++tries >= 5) { + console.warn('Failed to load common.js after 5 attempts'); + d.documentElement.style.visibility = 'visible'; + alert('Failed to load common.js. Please reload the page.'); + return; + } + setTimeout(loadFiles, Math.min(5000, 100 * tries)); + }; document.head.appendChild(l); })();wled00/data/settings_um.htm (1)
13-21: Bound retries with backoff for loader; add UX on failureMirror the improved loader pattern so this page doesn’t get stuck hidden if common.js can’t be fetched.
- (function loadFiles() { + (function loadFiles() { + let tries = 0; const l = document.createElement('script'); l.src = 'common.js'; l.onload = () => loadResources(['style.css'], S); // load style.css then call S() - l.onerror = () => setTimeout(loadFiles, 100); + l.onerror = () => { + if (++tries >= 5) { + console.warn('Failed to load common.js after 5 attempts'); + d.documentElement.style.visibility = 'visible'; + alert('Failed to load common.js. Please reload the page.'); + return; + } + setTimeout(loadFiles, Math.min(5000, 100 * tries)); + }; document.head.appendChild(l); })();wled00/data/settings_sec.htm (1)
9-16: Avoid infinite retry loop; add backoff and user feedbackHarden the loader to prevent an endlessly hidden UI if common.js fails.
- (function loadFiles() { + (function loadFiles() { + let tries = 0; const l = document.createElement('script'); l.src = 'common.js'; l.onload = () => loadResources(['style.css'], S); // load style.css then call S() - l.onerror = () => setTimeout(loadFiles, 100); + l.onerror = () => { + if (++tries >= 5) { + console.warn('Failed to load common.js after 5 attempts'); + d.documentElement.style.visibility = 'visible'; + alert('Failed to load common.js. Please reload the page.'); + return; + } + setTimeout(loadFiles, Math.min(5000, 100 * tries)); + }; document.head.appendChild(l); })();wled00/data/settings_sync.htm (1)
48-50: RedundantgetURLfunction definition.The
getURLfunction is defined locally on lines 48-50, but it's already provided bycommon.jswhich is loaded dynamically beforeS()is called (line 13). SincegetURLis only called from withinS()(lines 45-46) aftercommon.jshas loaded, this local definition appears redundant.Consider removing the local definition to avoid duplication:
-function getURL(path) { - return (loc ? locproto + "//" + locip : "") + path; -}Note: Verify that no other code paths call
getURLbeforecommon.jsloads.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
wled00/data/common.js(1 hunks)wled00/data/index.js(13 hunks)wled00/data/settings.htm(1 hunks)wled00/data/settings_2D.htm(2 hunks)wled00/data/settings_dmx.htm(2 hunks)wled00/data/settings_leds.htm(3 hunks)wled00/data/settings_sec.htm(2 hunks)wled00/data/settings_sync.htm(2 hunks)wled00/data/settings_time.htm(2 hunks)wled00/data/settings_ui.htm(2 hunks)wled00/data/settings_um.htm(2 hunks)wled00/data/settings_wifi.htm(2 hunks)wled00/ws.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- wled00/ws.cpp
🧰 Additional context used
📓 Path-based instructions (3)
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/common.jswled00/data/settings_dmx.htmwled00/data/settings_time.htmwled00/data/settings_um.htmwled00/data/settings_leds.htmwled00/data/settings_sec.htmwled00/data/settings_wifi.htmwled00/data/settings_sync.htmwled00/data/settings_2D.htmwled00/data/settings_ui.htmwled00/data/index.jswled00/data/settings.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/common.jswled00/data/settings_dmx.htmwled00/data/settings_time.htmwled00/data/settings_um.htmwled00/data/settings_leds.htmwled00/data/settings_sec.htmwled00/data/settings_wifi.htmwled00/data/settings_sync.htmwled00/data/settings_2D.htmwled00/data/settings_ui.htmwled00/data/index.jswled00/data/settings.htm
wled00/data/settings*.htm
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Name settings pages as settings*.htm within the web UI
Files:
wled00/data/settings_dmx.htmwled00/data/settings_time.htmwled00/data/settings_um.htmwled00/data/settings_leds.htmwled00/data/settings_sec.htmwled00/data/settings_wifi.htmwled00/data/settings_sync.htmwled00/data/settings_2D.htmwled00/data/settings_ui.htmwled00/data/settings.htm
🧠 Learnings (1)
📚 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/settings*.htm : Name settings pages as settings*.htm within the web UI
Applied to files:
wled00/data/settings.htm
🪛 Biome (2.1.2)
wled00/data/index.js
[error] 2815-2816: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ 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). (15)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (13)
wled00/data/settings_ui.htm (3)
7-7: LGTM: prevent FOUC while dependencies loadHiding html until resources load avoids flicker; loadResources() makes it visible on completion.
221-221: LGTM: onload removal aligns with deferred initInit via loader keeps ordering guarantees and avoids “xy is undefined” before common.js.
7-7: File already uses correct tab-based indentation per guidelinesThe indentation in
wled00/data/settings_ui.htmis already compliant with coding guidelines—all indented lines use tabs (^I), not spaces. Ensurenpm run buildis run after making any web UI modifications to regenerate embedded headers before firmware compilation.wled00/data/settings_time.htm (2)
7-7: LGTM: no-FUOC guard matches loadResources revealHiding html until resource load completes is appropriate here.
132-132: LGTM: init moved off body onloadConsistent with the new sequential loader flow.
wled00/data/settings_um.htm (2)
7-7: LGTM: initial hide to prevent unstyled flashPairs correctly with loadResources’ reveal.
284-284: LGTM: body onload removal aligns with deferred initKeeps init ordering consistent.
wled00/data/settings_sec.htm (2)
7-7: LGTM: FOUC prevention matches common.js revealConsistent with the shared loader behavior.
42-42: LGTM: loader-based init replaces body onloadKeeps dependency order deterministic.
wled00/data/settings_dmx.htm (1)
10-16: Verify infinite retry behavior is intentional.The
loadFilesIIFE will retry loadingcommon.jsindefinitely every 100ms on failure, whereas other data loading functions inindex.jslimit retries to 5 attempts. Under persistent network failures or misconfigurations, this could result in an infinite loop of retry attempts.Consider whether a retry limit (matching the 5-retry pattern used elsewhere) would be more appropriate:
-(function loadFiles() { +(function loadFiles(attempt = 0) { const l = document.createElement('script'); l.src = 'common.js'; l.onload = () => loadResources(['style.css'], S); // load style.css then call S() - l.onerror = () => setTimeout(loadFiles, 100); + l.onerror = () => { if (attempt < 5) setTimeout(() => loadFiles(attempt + 1), 100); }; document.head.appendChild(l); })();This pattern appears in all settings_*.htm files.
wled00/data/index.js (3)
279-294: Sequential initialization looks good, but verify error handling doesn't halt progression.The sequential loading pattern correctly addresses the low-heap/503-error issue mentioned in the PR objectives. Each async function properly resolves even on error, allowing initialization to continue.
One minor suggestion: Consider logging initialization progress for debugging:
(async ()=>{ try { + console.log('[WLED] Starting initialization...'); await loadPalettes(); // loads base palettes and builds #pallist (safe first) await loadFXData(); // loads fx data await loadFX(); // populates effect list await loadPalettesData(); // fills palettesData[] for previews await requestJson(); // fills lastinfo.cpalcount (safe now, pallist exists) populatePalettes(); // repopulate with custom palettes now that cpalcount is known await loadPresets(); // load presets last if (cfg.comp.css) await loadSkinCSS('skinCss'); + console.log('[WLED] Initialization complete'); if (!ws) makeWS(); } catch(e) { showToast("Init failed: " + e, true); + console.error('[WLED] Initialization error:', e); } })();
516-537: Well-structured Promise conversion with retry logic.The conversion from callback-based to Promise-based loading is clean and maintains the retry logic (5 attempts with 100ms intervals). The recursive retry pattern correctly preserves the Promise chain.
1693-1768: requestJson Promise conversion maintains backward compatibility well.The Promise-based refactoring preserves the WebSocket fallback logic and correctly handles the command parameter. The retry logic (up to 10 attempts for requestJson vs 5 for data loading) seems intentional for higher reliability of state changes.
Minor observation: The higher retry count (10) for
requestJsoncompared to data loading (5) appears deliberate for ensuring command reliability.
blazoncek
left a comment
blazoncek
left a comment
There was a problem hiding this comment.
I welcome the idea though I think it can be improved by reducing the code.
I did find a few oddities, too.
| } | ||
| const file = files[i++]; | ||
| const isCSS = file.endsWith('.css'); | ||
| const el = d.createElement(isCSS ? 'link' : 'script'); |
There was a problem hiding this comment.
Why did you choose to implement JS loading as that already have a function for that, loadJS().
There was a problem hiding this comment.
my function loads JS and CSS, I did not understand the purpose of the already implemented one. maybe they can be combined?
There was a problem hiding this comment.
I took a look at it and the two functions do server quite different purposes, unifying them does not really make sense and will just make the code more complex, at least that is how it looks to me.
wled00/data/index.js
Outdated
| if (t.validity.valid && command.transition==null) { | ||
| var tn = parseInt(t.value*10); | ||
| if (tn != tr) command.transition = tn; | ||
| async function requestJson(command=null) { |
There was a problem hiding this comment.
requestJson() is used on button press as well as other places in the code. I hope this does not break such use.
There was a problem hiding this comment.
Do you know how to test this?
There was a problem hiding this comment.
Except by clicking everything, IDK.
I did mostly generate this using a series of AI requests but must admit I do not understand all of it in detail. So very well possible some of the code can be streamlined or improved upon. I did test it extensively on all platforms though. |
|
Interesting idea to tweak the behaviour of the browser to make it a little more friendly to the microcontroller. If we had request queuing in the server this might be less of an issue, but until then this might be a good idea |
I did that with the release of 0.14. It did not go well. Perhaps this implementation is better but needs to be thoroughly tested on multiple devices, especially in captive portal mode. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/data/index.js (1)
485-513: Address maintainer feedback: Remove callback parameter.As noted by blazoncek in previous review comments, the
callbackparameter is no longer necessary with the Promise-based pattern. Additionally, the early return at line 495 doesn't return a Promise, which will cause issues for callers usingawait loadPresets().Apply this fix:
-async function loadPresets(callback = null) { - // 1st boot (because there is a callback) - if (callback && pmt == pmtLS && pmt > 0) { - // we have a copy of the presets in local storage and don't need to fetch another one - populatePresets(true); - pmtLast = pmt; - callback(); - return; - } - // afterwards - if (!callback && pmt == pmtLast) return; +async function loadPresets() { + // Check if already loaded + if (pmt == pmtLS && pmt > 0) { + populatePresets(true); + pmtLast = pmt; + return; + } + if (pmt == pmtLast) return; return new Promise((resolve) => { fetch(getURL('/presets.json'), {method: 'get'}) .then(res => res.status=="404" ? {"0":{}} : res.json()) .then(json => { pJson = json; pmtLast = pmt; populatePresets(); resolve(); }) .catch(() => { presetError(false); resolve(); }) - .finally(() => { - if (callback) setTimeout(callback, 99); - }); }); }Update call sites at lines 288 and 2540 to remove callback arguments.
Based on past review comments.
♻️ Duplicate comments (2)
wled00/data/index.js (2)
19-19: Duplicate: Global retry variable still causes race conditions.The change from
falseto0is appropriate for numeric retry counting, but the underlying race condition flagged in previous reviews remains unresolved. While the sequentialawaitpattern in lines 279-294 prevents some concurrent access, thesetTimeoutretry paths (lines 530, 552, 576, 845) can still fire concurrently and corrupt the shared retry counter.
2806-2831: Duplicate: Self-comparison bug and undefined lastinfo.vid.The issues flagged in previous reviews remain unresolved:
- Line 2814:
d.vid == d.vidself-comparison is always true- As blazoncek confirmed,
lastinfo.vidis undefined at this point in the loading sequenceThe simplest fix per previous discussion is to remove the ineffective version check and rely on the try-catch for corrupt data handling:
if (lsPalData) { try { var d = JSON.parse(lsPalData); - if (d && d.vid == d.vid) { + if (d && d.p) { palettesData = d.p; return resolve(); } } catch (e) {} }Based on past review comments and static analysis hints.
🧹 Nitpick comments (2)
wled00/data/index.js (2)
279-294: Sequential loading trades speed for reliability.The conversion to sequential resource loading addresses the stated goal of preventing 503 errors in low-heap conditions, and the ordering is correct (e.g.,
populatePalettes()at line 287 correctly followsrequestJson()at line 286 to ensurelastinfo.cpalcountis available).However, this approach significantly increases page load time compared to parallel fetching. Consider whether some independent resources (e.g.,
loadFXData()andloadPalettes()) could be safely parallelized usingPromise.all()while keeping dependent resources sequential.
2833-2851: Consider completing async/await conversion.
getPalettesData()still uses a callback parameter while most other loaders have been converted to pure Promise-based APIs. For consistency and to eliminate the callback/Promise mixing, consider converting this to:-function getPalettesData(page, callback) { +async function getPalettesData(page) { fetch(getURL(`/json/palx?page=${page}`), {method: 'get'}) .then(res => res.ok ? res.json() : Promise.reject()) .then(json => { retry = 0; palettesData = Object.assign({}, palettesData, json.p); - if (page < json.m) setTimeout(()=>{ getPalettesData(page + 1, callback); }, 75); - else callback(); + if (page < json.m) return new Promise(resolve => setTimeout(async () => { await getPalettesData(page + 1); resolve(); }, 75)); }) .catch((error)=>{ if (retry<5) { retry++; - setTimeout(()=>{getPalettesData(page,callback);}, 100); + return new Promise(resolve => setTimeout(async () => { await getPalettesData(page); resolve(); }, 100)); } else { showToast(error, true); - callback(); } }); }Then update the call at line 2822 to use
await getPalettesData(0).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/data/common.js(1 hunks)wled00/data/index.js(13 hunks)wled00/ws.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- wled00/ws.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/data/common.js
🧰 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/index.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/index.js
🧠 Learnings (4)
📚 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/data/index.js
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.js
📚 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: Manually validate the web UI after changes (load index, navigation, color controls, effects, settings) and check browser console for JS errors
Applied to files:
wled00/data/index.js
📚 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: WLED's heap panic recovery in the main loop intentionally prioritizes UI functionality over complete system reinitialization. Strip reinitialization may be deliberately skipped during heap panic as a last-resort measure to free enough memory to maintain a working UI.
Applied to files:
wled00/data/index.js
🧬 Code graph analysis (1)
wled00/data/index.js (1)
wled00/data/common.js (2)
l(104-104)ws(159-159)
🪛 Biome (2.1.2)
wled00/data/index.js
[error] 2814-2815: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ 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). (12)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- 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 (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
🔇 Additional comments (3)
wled00/data/index.js (3)
203-214: LGTM: Promise-based CSS loading.The conversion to a Promise-based pattern is well-implemented with early resolution if the CSS is already loaded and proper event handlers for success/failure cases.
1692-1767: Verify async conversion doesn't break interactive usage.As noted by blazoncek,
requestJson()is called on button presses and throughout the codebase. The async conversion maintains backward compatibility since non-awaited calls will still work, but verify that interactive features (buttons, sliders, color changes) still function correctly, especially any code paths that may have relied on synchronous behavior.Additionally,
requestJson()uses a retry limit of 10 (line 1758) while other loaders use 5. Consider standardizing the retry count across all functions for consistency.Manual testing required: Load the index page, test all interactive controls (power button, brightness slider, color picker, effect selection, preset loading) and verify no degradation in responsiveness.
279-294: Reminder: Regenerate embedded headers after changes.Per coding guidelines, after modifying web UI files under
wled00/data, runnpm run buildto regenerate the embedded headers before any firmware build.As per coding guidelines.
…ack in loadPresets() also udpated from main and added sequential loading to liveview files
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
wled00/data/liveviewws2D.htm (1)
17-24: Verify unlimited retry behavior for common.js loading.Same concern as in
liveview.htm(lines 21-28): the dynamic loader retries indefinitely on error without a maximum retry count. Ensure this unbounded retry behavior is intentional for this critical bootstrap dependency.wled00/data/index.js (1)
2810-2810: Critical bug: self-comparison makes version check ineffective.Line 2810 contains a self-comparison
d.vid == d.vidwhich is always true. Based on the past review discussion,lastinfo.vidis not available at this point in the loading sequence, so a version check against it is not feasible.Based on the past review discussion, remove the ineffective version check:
try { var d = JSON.parse(lsPalData); - if (d && d.vid == d.vid) { + if (d && d.p) { palettesData = d.p; return resolve(); } } catch (e) {}Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
wled00/data/common.js(2 hunks)wled00/data/index.js(12 hunks)wled00/data/liveview.htm(3 hunks)wled00/data/liveviewws2D.htm(1 hunks)wled00/data/settings.htm(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/data/settings.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/liveviewws2D.htmwled00/data/liveview.htmwled00/data/common.jswled00/data/index.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/liveviewws2D.htmwled00/data/liveview.htmwled00/data/common.jswled00/data/index.js
🧠 Learnings (6)
📚 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/liveviewws2D.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/data/index.js
📚 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/data/index.js
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.js
📚 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: Manually validate the web UI after changes (load index, navigation, color controls, effects, settings) and check browser console for JS errors
Applied to files:
wled00/data/index.js
📚 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: WLED's heap panic recovery in the main loop intentionally prioritizes UI functionality over complete system reinitialization. Strip reinitialization may be deliberately skipped during heap panic as a last-resort measure to free enough memory to maintain a working UI.
Applied to files:
wled00/data/index.js
🧬 Code graph analysis (2)
wled00/data/common.js (1)
wled00/data/index.js (11)
i(609-609)i(742-742)i(917-917)i(1449-1449)i(2191-2191)i(2198-2198)i(2215-2215)i(2798-2798)d(1382-1382)e(1477-1477)url(1391-1391)
wled00/data/index.js (1)
wled00/data/common.js (2)
l(107-107)ws(153-153)
🪛 Biome (2.1.2)
wled00/data/index.js
[error] 2810-2811: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ 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 (9)
wled00/data/liveview.htm (3)
21-28: Verify unlimited retry behavior for common.js loading.The dynamic loader retries indefinitely on error (line 26) without a maximum retry count. This differs from the bounded retry logic (retry count 1→5) used elsewhere in the PR (e.g.,
index.jslines 524-530, 546-552). While persistent retries may be acceptable for a critical bootstrap dependency likecommon.js, confirm this is intentional and won't cause infinite retry loops in failure scenarios (e.g., missing file, network issues).Consider adding a retry limit or timeout to prevent infinite retries:
// load common.js with retry on error - (function common() { + (function common(retries = 0) { const l = document.createElement('script'); l.src = 'common.js'; l.onload = () => S(); - l.onerror = () => setTimeout(common, 100); + l.onerror = () => { + if (retries < 10) setTimeout(() => common(retries + 1), 100); + else console.error('Failed to load common.js after 10 retries'); + }; document.head.appendChild(l); })();
66-85: LGTM! Startup flow is well-structured.The deferred initialization pattern (calling
S()aftercommon.jsloads) properly sequences resource loading. The WebSocket callback pattern on line 72 aligns with the updatedconnectWssignature incommon.js(lines 152-168), and error handling for message parsing is in place.
88-88: LGTM! Startup flow correctly delegated to dynamic loader.Removing the
onload="S()"attribute prevents double initialization now thatS()is invoked by the dynamic common.js loader (line 25). This is the correct approach for the new sequential loading pattern.wled00/data/common.js (1)
79-82: Verify unlimited retry behavior for individual resource loads.The error handler (line 79-82) retries failed resource loads indefinitely without a maximum retry count. While this ensures resources eventually load, it could cause infinite retry loops if a resource is permanently unavailable (e.g., deleted file, misconfiguration). Consider whether a bounded retry approach (as used in
index.jslines 524-530, 839-845) would be more robust.If bounded retries are preferred, apply this diff:
+ let retries = {}; const loadNext = () => { if (i >= files.length) { if (init) { d.documentElement.style.visibility = 'visible'; d.readyState === 'complete' ? init() : window.addEventListener('load', init); } return; } const file = files[i++]; + retries[file] = (retries[file] || 0); const isCSS = file.endsWith('.css'); const el = d.createElement(isCSS ? 'link' : 'script'); if (isCSS) { el.rel = 'stylesheet'; el.href = file; const st = d.head.querySelector('style'); if (st) d.head.insertBefore(el, st); else d.head.appendChild(el); } else { el.src = file; d.head.appendChild(el); } el.onload = () => { setTimeout(loadNext, 0);}; el.onerror = () => { - i--; // load this file again - setTimeout(loadNext, 100); + if (retries[file] < 10) { + i--; // load this file again + retries[file]++; + setTimeout(loadNext, 100); + } else { + console.error('Failed to load', file, 'after 10 retries'); + setTimeout(loadNext, 0); // continue with next file + } }; }; loadNext();wled00/data/liveviewws2D.htm (1)
32-61: LGTM! Startup flow is correct, pending fix in common.js.The
S()startup function properly initializes the canvas and WebSocket connection. The callback pattern on line 37 is correct and will work once the critical bug incommon.jsline 164 is fixed (see comment onwled00/data/common.js).wled00/data/index.js (4)
203-214: LGTM! Clean Promise-based CSS loading.The conversion of
loadSkinCSSto a Promise-based implementation is well done. The function properly resolves on successful load and rejects on error, enabling sequential loading viaawait.
279-294: LGTM! Sequential initialization properly implements PR objectives.The async IIFE correctly sequences resource loading to prevent "xy is undefined" errors caused by missing dependencies. The sequential
awaitpattern ensures palettes load before presets, FX data loads before effects populate, etc. Error handling via try/catch provides visibility into initialization failures.
1688-1762: LGTM! Async conversion maintains functionality with improved sequencing.The conversion to Promise-based flow is well-structured. The function properly handles both WebSocket and HTTP fetch paths, with appropriate retry logic (up to 10 attempts vs. 5 for other loaders—this may be intentional given
requestJson's criticality). The sequential loading of presets and WebSocket creation (lines 1740-1746) prevents race conditions.
2829-2847: LGTM! Palette pagination with retry logic is sound.The recursive pagination in
getPalettesDataproperly handles multi-page palette data with retry on failure. Although it uses the globalretryvariable, the sequential pagination (line 2835) and per-page retry logic (lines 2839-2845) prevent interference. The past review concern about retry counter races was marked as addressed, and the current implementation appears correct.
there is request queuing, but each queue eats up RAM and if heap is low, they do get dropped and throw a 503 so I dont think this can ever be resolved on the server side. |
- removed pmtLast - loadPresets() always loads the presets - on page load, presets are loaded from localStorage if controller was not rebooted
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/data/index.js (1)
502-523: Global retry variable still causes potential race conditions.Past reviews flagged that the global
retryvariable (line 19) is shared acrossloadPalettes,loadFX,loadFXData, andrequestJson. Although these functions are awaited sequentially during initialization (lines 282-286), their retry paths usesetTimeoutwhich can cause concurrent execution if retries are active when the next function starts.For example:
loadPalettesfails at T=0, schedules retry at T=100ms- Meanwhile,
loadFXruns at T=10 and fails, increments the sameretrycounter- The retry counter state is now corrupted between the two functions
A previous review comment indicated this was "Addressed in commits 30fbf55 to 31618c2", but the current code still uses the global
retryvariable.Consider making
retrya parameter to each function or using a closure/local variable to avoid interference.
🧹 Nitpick comments (2)
wled00/data/index.js (2)
279-295: Sequential initialization looks solid; consider more specific error reporting.The async IIFE correctly sequences all initialization steps to avoid concurrent requests. However, since most individual loaders always resolve (never reject), the catch block may not capture intermediate failures. Consider logging which step failed or allowing individual steps to reject for better diagnostics.
Additionally, the
populatePresets(true)call on line 288 will recursively callloadPresets()ifpJsonis falsy (line 589), which means theelse await loadPresets()on line 289 might become redundant in that case. This logic works but could be clearer.
2809-2827: Consider removing callback parameter in favor of Promise pattern.As noted in a previous review comment, the
callbackparameter is no longer necessary sinceloadPalettesData(line 2782) now returns a Promise. Consider refactoringgetPalettesDatato return a Promise and use async/await for cleaner pagination logic.This would also make the retry handling more consistent with other loader functions and eliminate the callback pattern mixing with Promise-based code.
Based on past review suggestions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(15 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/index.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/index.js
🧠 Learnings (4)
📚 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/index.js
📚 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/data/index.js
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.js
📚 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: WLED's heap panic recovery in the main loop intentionally prioritizes UI functionality over complete system reinitialization. Strip reinitialization may be deliberately skipped during heap panic as a last-resort measure to free enough memory to maintain a working UI.
Applied to files:
wled00/data/index.js
🧬 Code graph analysis (1)
wled00/data/index.js (1)
wled00/data/common.js (2)
l(107-107)ws(153-153)
🪛 Biome (2.1.2)
wled00/data/index.js
[error] 2790-2791: Comparing to itself is potentially pointless.
(lint/suspicious/noSelfCompare)
⏰ 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). (14)
- 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 (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- 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 (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (1)
wled00/data/index.js (1)
1678-1743: Async conversion is compatible with all interactive use cases.Verification confirms that all interactive call sites—including button presses (lines 1756, 1769, 1779), toggles (selSegEx, selSeg, setRev, setRevY), and slider changes—follow a fire-and-forget pattern: they invoke
requestJson(obj)and return immediately without awaiting or checking the result. This pattern is fully compatible with async/Promise behavior.The existing codebase already uses Promise semantics in requestJson calls (line 286 with
await, line 1736 with.then().catch()), confirming the function was already async. No call site depends on synchronous completion, so the conversion introduces no breaking changes.Note: The global
retryvariable race condition with different limits (5 vs. 10) is pre-existing and not specific to this PR.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
wled00/data/index.js (3)
1676-1740: Note: Retry counter issue also affectsrequestJson.The same retry counter reset issue documented in lines 501-521 affects this function. Line 1677 declares
let retry = 0, but line 1734's recursive callrequestJson(command)creates a new scope with retry reset to 0, preventing the retry limit from working (line 1732 checksretry<10).
2806-2824: Retry counter reset issue affects pagination logic.Same retry issue as other loading functions. Additionally, the pagination recursion (line 2812) and retry recursion (line 2818) both call
getPalettesData(page, callback), but the retry counter resets on each call, so retry failures during pagination would retry infinitely.
2780-2803: Potential race condition:lastinfo.vidmay be undefined during early initialization.The version check at line 2787 compares
d.vid == lastinfo.vid, but as noted in past reviews,lastinfo.vidmay not be available whenloadPalettesData()is called early in the initialization sequence (line 284, beforerequestJson()at line 285 populateslastinfo).Consider either:
- Removing the version check and relying solely on the try-catch to validate cached data structure
- Adding a null check:
if (d && d.vid && lastinfo.vid && d.vid == lastinfo.vid)- Moving palette data loading after
requestJson()completesApply this defensive fix:
try { var d = JSON.parse(lsPalData); - if (d && d.vid == lastinfo.vid) { + if (d && d.p && lastinfo && lastinfo.vid && d.vid == lastinfo.vid) { palettesData = d.p; return resolve(); } } catch (e) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(15 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/index.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/index.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/index.js
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.js
📚 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: WLED's heap panic recovery in the main loop intentionally prioritizes UI functionality over complete system reinitialization. Strip reinitialization may be deliberately skipped during heap panic as a last-resort measure to free enough memory to maintain a working UI.
Applied to files:
wled00/data/index.js
🧬 Code graph analysis (1)
wled00/data/index.js (1)
wled00/data/common.js (2)
l(107-107)ws(153-153)
⏰ 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). (6)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
🔇 Additional comments (4)
wled00/data/index.js (4)
202-213: LGTM: Clean Promise-based CSS loader.The conversion to Promise-based loading is well-structured with proper error handling via
reject, early exit if the resource is already loaded, and appropriate use ofonload/onerrorhandlers.
2927-2927: LGTM: Proper focus state check.The blur event guard correctly uses
d.activeElementandd.hasFocus()to prevent hiding the filter UI when focus remains within the component.
3174-3247: Consistent use of document alias.The DOM manipulation throughout the
simplifyUI()function correctly uses thedvariable (defined at line 17 asdocument), consistent with the established codebase pattern.
278-294: No changes needed—populatePalettes() is synchronous.The
populatePalettes()function (lines 946–980) contains only synchronous operations: array manipulation, string building, and DOM manipulation viainnerHTMLandclassList. It does not return a Promise or perform any async work, so awaiting it at line 286 is unnecessary. The code is correct as written.
wled00/data/index.js
Outdated
| async function populatePresets(fromls) | ||
| { | ||
| if (fromls) pJson = JSON.parse(localStorage.getItem("wledP")); | ||
| if (!pJson) {setTimeout(loadPresets,250); return;} | ||
| if (!pJson) {await loadPresets(); return;} |
There was a problem hiding this comment.
Verify recursive call behavior in populatePresets.
Line 587 calls await loadPresets() recursively if pJson is null, which then calls populatePresets() again (line 491). This could lead to an infinite loop if loadPresets() fails silently and returns without setting pJson.
Verify that loadPresets() always sets pJson to at least an empty object (line 490 shows it does via res.status=="404" ? {"0":{}} : res.json()), so the guard should work. However, the .catch() block (line 494) calls presetError(false) but still resolves, meaning pJson remains undefined and line 587 would loop infinitely.
Apply this fix to prevent infinite recursion:
async function populatePresets(fromls)
{
if (fromls) pJson = JSON.parse(localStorage.getItem("wledP"));
- if (!pJson) {await loadPresets(); return;}
+ if (!pJson) {
+ await loadPresets();
+ if (!pJson) pJson = {}; // prevent infinite recursion if loadPresets fails
+ return;
+ }🤖 Prompt for AI Agents
In wled00/data/index.js around lines 584-587, populatePresets currently does "if
(!pJson) {await loadPresets(); return;}" which can trigger infinite recursion if
loadPresets() fails to set pJson; change the flow so after awaiting
loadPresets() you re-read and validate pJson and bail out safely rather than
returning into a caller that will re-invoke populatePresets. Concretely: after
await loadPresets(), set pJson = JSON.parse(localStorage.getItem("wledP")) (or
default to an empty object) and if it is still falsy return without calling
populatePresets again (or throw an error) so no recursive loop can occur.
There was a problem hiding this comment.
does presetError() not result in a valid pJson? that is its purpose
There was a problem hiding this comment.
@DedeHai, looking at the code, presetError() does not set pJson to a valid value. Its purpose is to display an error message in the UI, not to initialize the variable.
Here's what presetError() actually does (lines ~323-349):
function presetError(empty)
{
var hasBackup = false; var bckstr = "";
try {
bckstr = localStorage.getItem("wledP");
if (bckstr.length > 10) hasBackup = true;
} catch (e) {}
var cn = `<div class="pres c" ...>`;
// ... builds error message HTML ...
gId('pcont').innerHTML = cn; // <-- Only modifies DOM
if (hasBackup) gId('bck').value = bckstr;
}It only manipulates the DOM to show an error message (and optionally a backup restore UI). It never assigns anything to pJson.
So when loadPresets() fails:
- The
.catch()block callspresetError(false) pJsonremainsundefined- The promise resolves anyway
- Back in
populatePresets(), afterawait loadPresets(),pJsonis still falsy
While the immediate return prevents stack overflow, CodeRabbit's concern is valid: if anything retries calling populatePresets() (e.g., user interaction or a retry mechanism), it will attempt to load presets again, fail again, and repeat the cycle without ever establishing a valid pJson state.
CodeRabbit's suggestion to add a fallback initialization (if (!pJson) pJson = {}) after the await would prevent that scenario and ensure pJson is always defined after populatePresets() completes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
wled00/data/index.js (2)
578-581: Critical: Potential infinite recursion remains unfixed.A past review (still in comments) identified that line 581 can cause infinite recursion if
loadPresets()fails (non-404 error). The.catch()block at lines 494-496 callspresetError(false)and resolves without settingpJson, so repeated calls topopulatePresets(fromls)will loop indefinitely.Apply this fix to prevent infinite recursion:
async function populatePresets(fromls) { if (fromls) pJson = JSON.parse(localStorage.getItem("wledP")); - if (!pJson) {await loadPresets(); return;} + if (!pJson) { + await loadPresets(); + if (!pJson) pJson = {}; // prevent infinite recursion + return; + }
2772-2796: Version check may fail if lastinfo not yet populated.The past self-comparison bug (
d.vid == d.vid) appears fixed tod.vid == lastinfo.vidat line 2779. However, per previous review discussion,lastinfo.vidmay be undefined whenloadPalettesData()executes. The initialization sequence (line 284) callsloadPalettesData()beforerequestJson(), which populateslastinfo.Add defensive checks:
try { var d = JSON.parse(lsPalData); - if (d && d.vid == lastinfo.vid) { + if (d && d.vid && lastinfo && lastinfo.vid && d.vid === lastinfo.vid) { palettesData = d.p; return resolve(); } } catch (e) {}Alternatively, consider removing the version check entirely (as suggested in past review) since timing makes it unreliable.
🧹 Nitpick comments (1)
wled00/data/index.js (1)
2798-2814: Consider modernizing pagination to Promise-based pattern.The retry logic is correctly implemented with a parameter. However, the callback pattern (while functional) is inconsistent with the Promise-based conversions elsewhere in this file.
Consider converting to a Promise-based recursive pagination:
-function getPalettesData(page, callback, retry=0) { +function getPalettesData(page, retry=0) { + return new Promise((resolve, reject) => { fetch(getURL(`/json/palx?page=${page}`), {method: 'get'}) .then(res => res.ok ? res.json() : Promise.reject()) .then(json => { palettesData = Object.assign({}, palettesData, json.p); - if (page < json.m) setTimeout(()=>{ getPalettesData(page + 1, callback); }, 75); - else callback(); + if (page < json.m) { + setTimeout(()=>{ getPalettesData(page + 1, retry).then(resolve).catch(reject); }, 75); + } else { + resolve(); + } }) .catch((error)=>{ if (retry<5) { - setTimeout(()=>{getPalettesData(page,callback,retry+1);}, 100); + setTimeout(()=>{getPalettesData(page,retry+1).then(resolve).catch(reject);}, 100); } else { showToast(error, true); - callback(); + resolve(); // or reject(error) } }); + }); }Then update the caller at line 2787:
- getPalettesData(0, () => { + getPalettesData(0).then(() => { localStorage.setItem("wledPalx", JSON.stringify({ p: palettesData, vid: lastinfo.vid })); redrawPalPrev(); - setTimeout(resolve, 99); + resolve(); });Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(15 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/index.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/index.js
🧠 Learnings (6)
📚 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/index.js
📚 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/data/index.js
📚 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/index.js
📚 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/data/index.js
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.js
📚 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: WLED's heap panic recovery in the main loop intentionally prioritizes UI functionality over complete system reinitialization. Strip reinitialization may be deliberately skipped during heap panic as a last-resort measure to free enough memory to maintain a working UI.
Applied to files:
wled00/data/index.js
🧬 Code graph analysis (1)
wled00/data/index.js (1)
wled00/data/common.js (2)
l(107-107)ws(153-153)
⏰ 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). (4)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
🔇 Additional comments (11)
wled00/data/index.js (11)
202-213: LGTM: Clean Promise-based CSS loader.The conversion to a Promise-based pattern is well-structured with proper early return, error handling, and DOM manipulation.
485-499: LGTM: Async loadPresets follows consistent pattern.The Promise-based conversion correctly handles the 404 case with a default preset object and ensures resolution even on errors, maintaining UI stability.
501-519: LGTM: Retry logic correctly implemented with local parameter.The conversion addresses previous race condition concerns by using a function parameter instead of a global variable. The retry limit of 5 with 100ms intervals aligns with PR objectives for improved reliability.
521-539: LGTM: Consistent retry implementation.The async conversion and retry logic follow the same pattern as
loadPalettes, maintaining consistency across loader functions.
541-561: LGTM: Retry pattern consistent.The async conversion and retry logic are correctly implemented. The array manipulation (shift/unshift) appears intentional for effect data processing.
1670-1733: Verify intentional higher retry limit.The async conversion and retry logic are correctly implemented. However,
requestJson()uses a retry limit of 10 (line 1725) while other loaders use 5. Confirm this is intentional—higher retries may be warranted for critical state updates, but could also prolong failure recovery.
2506-2506: LGTM: Delayed preset reload after save.The 750ms delay allows the server to persist the preset before reloading, and the fire-and-forget pattern is appropriate here since
saveP()is not async.
452-452: LGTM: Async function call from onclick handler.Calling the async
loadPresets()from an onclick handler works correctly; the returned Promise is appropriately ignored for this user-initiated action.
2917-2917: LGTM: DOM manipulations use correct API.All DOM manipulations correctly use the
dalias (defined line 17 asvar d = document) and follow standard patterns.Also applies to: 3164-3177, 3225-3225, 3237-3237
27-27: The review comment is based on incorrect assumptions and should be disregarded.The original concern about variable shadowing is a false positive. Investigation reveals that:
index.htmlloads onlyindex.jsand does NOT loadcommon.jscommon.jsis loaded separately in other pages (settings, liveview, etc.), but not inindex.html- The
wsvariable incommon.js:153is a local variable inside theconnectWs()function, not a global declaration- The
wsvariable inindex.js:27is global only within theindex.htmlcontextSince the two files are never loaded in the same scope, there is no shadowing, conflict, or collision between these variables. They operate independently in their respective contexts.
Likely an incorrect or invalid review comment.
278-294: The sequential initialization timing is correct; no changes needed.The original concern about
pmttiming can be dismissed. Here's why:
requestJson()returns a Promise that doesn't resolve until after the.then()chain completes. Critically,parseInfo(json.info)is called synchronously within the chain (line 1714) and assignspmt = i.fs.pmt(line 633) beforeresolve()is called (line 1721). This meansawait requestJson()on line 285 will not return untilpmtis fully assigned, so the conditional check at line 287 (if(pmt == pmtLS)) operates on a properly populated value.Edge cases where info is not in the response (WebSocket fast-path,
json.success, or empty responses) simply leavepmtat its initialized value (1 from line 24), which is harmless.
|
Hi @DedeHai Did you make additional updates on your side? |
|
see individual commits. |
|
What testing is required @DedeHai ? Could you add some notes, it seems like at least one person has already treated and confirmed the improvement, so what it still to be tested? |
|
@netmindz from my own experience these kind of changes need thorough testing on various mobile devices (especially Android) in captive portal mode. |
|
@netmindz I ran some basic tests, also in AP mode and fixed some issues afterwards but I still want to re-test it, also on ESP8266 and also on android, firefox, safari. after those are successful, we can dare to merge this for further testing. edit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wled00/data/index.js (2)
485-499: Consider awaiting populatePresets for consistent completion.Line 491 calls
populatePresets()withoutawait, even though it's now anasyncfunction. This creates a "fire and forget" call where the Promise resolution on line 492 happens before preset population completes.If the caller of
loadPresets()expects presets to be fully populated when the Promise resolves (e.g., in the initialization sequence), this could cause race conditions. Consider whether line 491 should be:- populatePresets(); - resolve(); + await populatePresets(); + resolve();or if the current behavior is intentional for performance reasons, add a comment explaining why.
2799-2815: Consider removing callback parameter per past review.The function signature still includes a
callbackparameter, which blazoncek suggested removing in a past review comment. While the current implementation works correctly for pagination, consider refactoring to return a Promise and use async/await:async function getPalettesData(page = 0, retry = 0) { try { const json = await fetch(getURL(`/json/palx?page=${page}`)).then(res => res.ok ? res.json() : Promise.reject()); palettesData = Object.assign({}, palettesData, json.p); if (page < json.m) { await new Promise(resolve => setTimeout(resolve, 75)); await getPalettesData(page + 1, retry); } } catch (error) { if (retry < 5) { await new Promise(resolve => setTimeout(resolve, 100)); await getPalettesData(page, retry + 1); } else { showToast(error, true); } } }Then update the caller on line 2788 to
await getPalettesData(0);.Based on past review feedback from blazoncek.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(16 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/index.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/index.js
🧠 Learnings (8)
📓 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: 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.
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/** : When modifying web UI files, run `npm run build` to regenerate embedded headers before any firmware build
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
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)
📚 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/index.js
📚 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/data/index.js
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.js
📚 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/data/index.js
📚 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/data/index.js
📚 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/index.js
📚 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: WLED's heap panic recovery in the main loop intentionally prioritizes UI functionality over complete system reinitialization. Strip reinitialization may be deliberately skipped during heap panic as a last-resort measure to free enough memory to maintain a working UI.
Applied to files:
wled00/data/index.js
🧬 Code graph analysis (1)
wled00/data/index.js (1)
wled00/data/common.js (2)
l(107-107)ws(153-153)
⏰ 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). (9)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (usermods)
- 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 (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
🔇 Additional comments (8)
wled00/data/index.js (8)
278-294: Sequential initialization flow looks correct.The sequential loading pattern (await each step) should significantly improve reliability on low-heap devices by avoiding parallel request overload. The error handling with a single catch block is appropriate for initialization failures.
However, line 287 calls
populatePresets(true)withoutawait, while line 288 doesawait loadPresets(). This is intentional sincepopulatePresetswithfromls=truereads from localStorage (synchronous), but the function signature is nowasync. Consider whether line 287 should also be awaited for consistency, or document why it's not needed.
501-561: LGTM - Retry logic correctly implemented.The retry parameter pattern for
loadPalettes,loadFX, andloadFXDatacorrectly addresses the previous issues with global retry counters and reset-on-recursion. Each function:
- Tracks its own retry count via parameter
- Retries up to 5 times with 100ms delay
- Shows error toast on final failure but still resolves to allow initialization to continue
This should significantly improve reliability on poor Wi-Fi connections.
1671-1734: Verify requestJson callers handle async change correctly.The conversion of
requestJsonto async with retry logic looks correct, but note:
Retry count difference: This function retries up to 10 times (line 1726), while other loaders retry only 5 times. This may be intentional since user-triggered commands are more important, but verify this is the desired behavior.
Caller impact: This function is called from many UI interaction handlers (togglePower, setFX, setPalette, etc.). The call sites don't
awaitthe result, treating it as "fire and forget." This should be acceptable for most UI operations, but verify that critical operations (e.g., preset saves, segment changes) don't need to wait for completion or handle errors.Based on past review concerns from blazoncek about breaking button press usage.
452-452: LGTM - Function call syntax corrected.The
onclickhandler now correctly includes()to callloadPresets(). This ensures the function executes when the user clicks to retry loading presets after an error.
202-213: LGTM - loadSkinCSS Promise conversion.The conversion to return a Promise is clean and correct:
- Early return if CSS already loaded (line 204)
- Proper onload/onerror handlers for Promise resolution (lines 209-210)
- Integrates well with the sequential initialization flow
866-866: LGTM - Segment count guard clause.The early return when
segCount === 0prevents unnecessary processing and potential errors from iterating over an empty segment list.
3165-3238: LGTM - Consistent document alias usage.The uses of
d.createElement(lines 3165, 3178, 3226) are consistent with the document alias defined on line 17. The code properly uses this alias throughout for DOM manipulation.
578-581: Review comment is incorrect.The infinite recursion concern is unfounded.
pJsonis initialized to{}at line 21 (var pJson = {}, eJson = {}, lJson = {};), notundefined. Since an empty object{}is truthy in JavaScript, the conditionif (!pJson)on line 581 evaluates tofalseandloadPresets()is never called. Even ifloadPresets()fails and callspresetError(false),pJsonremains as{}(truthy), preventing any retry loop on subsequent calls topopulatePresets().Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
wled00/data/index.js (2)
485-499: Loose equality operator with mixed types.Line 488 uses
res.status=="404"comparing a number to a string. While JavaScript's type coercion makes this work, use strict equality for clarity:-.then(res => res.status=="404" ? {"0":{}} : res.json()) +.then(res => res.status===404 ? {"0":{}} : res.json())
2773-2815: Palette data loading correctly implements pagination with retries.The Promise-based
loadPalettesDataproperly wraps the callback-basedgetPalettesDatapagination. The version check at line 2780 now works correctly becauserequestJson()is called earlier in the init sequence (line 284), ensuringlastinfo.vidis populated.The hybrid callback/Promise pattern in
getPalettesDatais acceptable given the pagination requirements, and error handling correctly allows partial data on failure (line 2812), maintaining UI functionality.Minor: The 99ms delay at line 2794 seems arbitrary; consider removing if unnecessary or documenting why it's needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(16 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/index.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/index.js
🧠 Learnings (14)
📓 Common learnings
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.
📚 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/data/index.js
📚 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/index.js
📚 Learning: 2025-09-12T17:29:43.826Z
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Applied to files:
wled00/data/index.js
📚 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/data/index.js
📚 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/data/index.js
📚 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/index.js
📚 Learning: 2025-11-14T13:37:11.994Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with appropriate defined constants when those constants are meaningful in the context of the PR. For example, the hardcoded value 32 should be replaced with WLED_MAX_SEGNAME_LEN when it represents a segment name length limit. This improves code maintainability and reduces the risk of inconsistencies.
Applied to files:
wled00/data/index.js
📚 Learning: 2025-04-30T05:41:03.633Z
Learnt from: blazoncek
Repo: wled/WLED PR: 4667
File: usermods/user_fx/user_fx.cpp:27-30
Timestamp: 2025-04-30T05:41:03.633Z
Learning: In WLED, the Segment::allocateData() method already includes optimization to check if data is allocated and sufficiently sized, handling buffer reuse to reduce memory fragmentation. Adding an external check like `if (SEGENV.data == nullptr && !SEGENV.allocateData(dataSize))` is unnecessary and could be problematic, as it bypasses proper size verification.
Applied to files:
wled00/data/index.js
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist in the codebase. For example, suggest replacing hardcoded "32" with WLED_MAX_SEGNAME_LEN if the context relates to segment name length limits.
Applied to files:
wled00/data/index.js
📚 Learning: 2025-11-14T13:37:30.955Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:30.955Z
Learning: In WLED code reviews, verify that file operations (especially file.open()) respect LittleFS filename limitations. Assume default WLED configuration with LittleFS default filename limit of 255 bytes. Do not assume extreme configuration values like WLED_MAX_SEGNAME_LEN = 512 which would not be standard configurations.
Applied to files:
wled00/data/index.js
📚 Learning: 2025-11-14T13:22:20.525Z
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:22:20.525Z
Learning: In WLED's image_loader.cpp and similar contexts, when suggesting cleanup or de-allocation code in error handlers, first verify whether the underlying library or function already handles cleanup after failure. For example, `new` expressions that throw exceptions do not assign partial pointers and automatically clean up storage, making explicit dealloc() calls in catch blocks unnecessary. Only suggest "safety" cleanup when the library/function's documented semantics do not already provide graceful failure handling.
Applied to files:
wled00/data/index.js
📚 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/data/index.js
📚 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: WLED's heap panic recovery in the main loop intentionally prioritizes UI functionality over complete system reinitialization. Strip reinitialization may be deliberately skipped during heap panic as a last-resort measure to free enough memory to maintain a working UI.
Applied to files:
wled00/data/index.js
🧬 Code graph analysis (1)
wled00/data/index.js (1)
wled00/data/common.js (2)
l(107-107)ws(153-153)
⏰ 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 (usermods)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
🔇 Additional comments (5)
wled00/data/index.js (5)
202-213: LGTM: Clean Promise-based resource loading.The conversion of
loadSkinCSSto return a Promise is well-implemented with proper early-exit for already-loaded resources and correct error handling.
278-294: Sequential initialization improves low-heap stability.The new Promise-based sequential loading correctly enforces resource order to prevent concurrent requests that could exhaust heap. The flow is logical: base data (palettes, effects) → device state → preview data → presets → optional resources (CSS, WebSocket).
The fire-and-forget IIFE pattern is acceptable here since the UI remains functional during progressive loading, and any errors are caught and displayed via toast.
501-561: Retry logic correctly implemented with local counters.Each loader (palettes, effects, effect data) now maintains its own retry counter as a parameter, eliminating race conditions from the previous global retry variable. The 5-retry limit with 100ms backoff is appropriate for transient network/heap issues, and graceful degradation (resolve on exhaustion) ensures initialization completes even if individual resources fail.
1671-1734: Async conversion maintains backward compatibility.The conversion of
requestJsonto async is well-executed. The function correctly supports both fire-and-forget calls (existing button handlers) and awaited calls (initialization sequence). All existing call sites use it as a statement without capturing the return value, so returning a Promise doesn't break functionality.The retry logic (max 10 retries vs 5 for loaders) and graceful degradation (resolves after exhaustion) are appropriate for the interactive command path.
Based on past review discussions with blazoncek.
278-294: Sequential loading architecture effectively addresses low-heap stability concerns.The PR successfully converts the initialization flow from parallel to sequential with the following improvements:
- Eliminated race conditions: Local retry parameters prevent concurrent retry counters from interfering
- Predictable resource loading: Sequential await chain ensures dependencies are met (e.g.,
lastinfo.vidavailable before palette cache check)- Graceful degradation: All loaders resolve on failure, allowing initialization to continue with partial functionality
- Backward compatibility:
requestJson()works for both awaited (init) and fire-and-forget (button handlers) usage patterns- Appropriate retry policies: 5 retries for non-critical loaders, 10 for interactive commands
The 100ms retry delay and increased retry counts align with the PR's goal of improving stability under poor Wi-Fi conditions without causing noticeable slowdown.
Also applies to: 1671-1734
|
@netmindz I did test this and found a few oddities I was later not able to reproduce. the last commit seems to solve #5080 but needs confirmation. edit: |
|
just found one small bug I am not able to fix: when saving any of the settings pages, the "xxx settings saved..." sometimes shows up with no CSS styling i.e. in white. But not always. Any ideas on why that is and how to fix it? edit: |
DedeHai
added
connectivity
4 participants
This greatly improves UI stability in low heap as well as bad wifi signal conditions and does not noticeably slow down any UI access.
I also increased the retries on failed requests from 1 to 5 but shortened the request interval so some requests may even be faster than before.
Combined with #4939 this makes the ESP8266 work flawlessly, I was even able to increase the number of LEDs from current 720 to 900 and its still more stable than it is currently. With this PR the UI keeps running down to about 6k of free heap whereas it is already struggeling at 10k without these changes.
the changes add ~900bytes to flash usage.
Summary by CodeRabbit
New Features
Bug Fixes
Chores