WLED icon indicating copy to clipboard operation
WLED copied to clipboard

Use sequential loading and requests for all UI resources

Open DedeHai opened this issue 4 months ago • 20 comments

  • Load common.js and style.css sequentially for all config pages (no more "xy is undefined" due to missing common.js)
  • Restrict all requests in index.js to single connection

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

    • Dynamic resource loader for sequential CSS/JS loading with retry and a new loadResources API
    • WebSocket connector now reuses an open socket and provides the socket to connection callbacks
  • Bug Fixes

    • Page is hidden until resources finish loading to avoid flashes of unstyled content
    • Bounded retry logic and centralized error handling to improve startup resilience
  • Chores

    • Startup refactored to async/Promise flows for more reliable initialization
    • Removed a redundant pre-flight memory check in WebSocket send logic

✏️ Tip: You can customize this high-level summary in your review settings.

DedeHai avatar Oct 19 '25 09:10 DedeHai

Walkthrough

Adds 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 wled00/data/index.js to async/Promise flows; updates WebSocket reuse/handshake in wled00/data/common.js; removes an ESP8266 pre-check in wled00/ws.cpp; minor CSS visibility change.

Changes

Cohort / File(s) Summary
Resource loader & WebSocket
\wled00/data/common.js``
Adds loadResources(files, init) to sequentially load JS/CSS with retry, reveal the document after load, and invoke an init callback; updates connectWs(onOpen) to reuse top.window.ws when OPEN, call getLoc() before URL formation, pass the WebSocket into onOpen, and return the WebSocket; removes storing ws on top.window.ws.
Async init & data flow refactor
\wled00/data/index.js``
Converts many callback flows to async/Promise-based functions (loadPalettes, loadFX, loadFXData, loadPalettesData, requestJson, loadSkinCSS, populatePresets, loadPresets, etc.), adds retry handling and caching, and centralizes initialization into a sequential async startup path with unified error handling.
Settings pages — dynamic loader + hide-until-ready
Settings HTMLs
\wled00/data/settings.htm``, \wled00/data/settings_2D.htm``, \wled00/data/settings_dmx.htm``, \wled00/data/settings_leds.htm``, \wled00/data/settings_sec.htm``, \wled00/data/settings_sync.htm``, \wled00/data/settings_time.htm``, \wled00/data/settings_ui.htm``, \wled00/data/settings_um.htm``, \wled00/data/settings_wifi.htm``
Replace static <script src="common.js"> and inline @import style usage with IIFE-based dynamic loaders that retry on error, call loadResources(['style.css'], S) (or equivalent) after loading, remove body onload="S()", and add CSS to hide the page until resources are ready.
Liveview pages — loader & WS callback update
\wled00/data/liveview.htm``, \wled00/data/liveviewws2D.htm``
Switch to dynamic loading of common.js, remove body onload, update WebSocket usage to connectWs(ws => ...), add bounded retry for HTTP updates, and consolidate 2D liveview startup into a single S() flow with message handling inside that flow.
WebSocket memory check change (ESP8266)
\wled00/ws.cpp``
Removes an early heap-availability pre-check in sendDataWs() (ESP8266) that compared payload length to a cached heap value; buffer allocation and subsequent allocation-failure handling remain.
CSS visibility tweak
\wled00/data/index.css``
Removes .expanded .frz from a combined selector that hid elements under .hide / .expanded, allowing .expanded .frz to remain visible unless controlled elsewhere.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Heterogeneous changes: async refactor in JS, API/contract change in common.js, many HTML edits, and a native C++ memory-check removal.
  • Areas needing extra attention:
    • wled00/data/index.js: verify async sequencing, retry semantics, localStorage caching, and compatibility with callers expecting previous callback signatures.
    • wled00/data/common.js: confirm loadResources retry/backoff, CSS insertion order, document reveal timing, and changed connectWs callback contract.
    • wled00/ws.cpp: ensure removing the ESP8266 pre-check doesn't alter behavior under low-memory conditions.
    • Representative updated HTML pages (e.g., settings_ui.htm, liveviewws2D.htm) to validate initialization timing and hidden/unhidden UI behavior.

Possibly related PRs

  • wled/WLED#4997 — Overlaps connectWs / common.js WebSocket logic changes; likely code-level overlap.
  • wled/WLED#4791 — Modifies heap-check logic in sendDataWs / ws.cpp; related to the removed ESP8266 pre-check.
  • wled/WLED#4914 — Edits FX data/UI that may interact with the refactored async FX/palette loading in index.js.

Suggested reviewers

  • netmindz
  • blazoncek
  • willmmiles

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 17.86% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: sequential loading and requests for UI resources, which directly aligns with the changeset's core objectives of preventing resource loading race conditions and implementing sequential request handling.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 19 '25 09:10 coderabbitai[bot]

I did find a few oddities, too.

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.

DedeHai avatar Oct 20 '25 17:10 DedeHai

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

netmindz avatar Oct 20 '25 18:10 netmindz

tweak the behaviour of the browser

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.

blazoncek avatar Oct 20 '25 18:10 blazoncek

If we had request queuing in the server this might be less of an issue, but until then this might be a good idea

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.

DedeHai avatar Nov 13 '25 07:11 DedeHai

Hi @DedeHai

I’ve tested it, and the issue #5077 has indeed been resolved!

Hopefully, this branch will be merged soon to improve performance.

happysmartlight avatar Nov 13 '25 08:11 happysmartlight

Hi @DedeHai

Did you make additional updates on your side? What does it improve? What was the reason or motivation behind these new changes?

happysmartlight avatar Nov 14 '25 03:11 happysmartlight

see individual commits.

DedeHai avatar Nov 14 '25 05:11 DedeHai

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 avatar Nov 15 '25 10:11 netmindz

@netmindz from my own experience these kind of changes need thorough testing on various mobile devices (especially Android) in captive portal mode. Unfortunately one can only have so many.

blazoncek avatar Nov 15 '25 10:11 blazoncek

@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: as for tests required, I really don't know, the "dangerous" changes are exclusively in index.js and the one bug that stands (palette preview not loaded in AP mode) is already present before this PR so I did not further investigate it yet. Things to look out for would be errors on high-wifi traffic / low wifi signal which I do not know how to emulate. Other than that I just poke around the UI, adding segments, presets, palettes etc. and see if it all works.

DedeHai avatar Nov 15 '25 11:11 DedeHai

@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. the biggest issue I found was on android the peek was not working at all and all the config pages were just black. Debugging showed that in the config pages, the "hidden" style element was not removed, leaving the pages blank. I then tried a different android device and it just worked. A full page refresh then fixed it. So I suspect the issue was a cached "common.js" which was lacking the init function that removes the "hidden" tag. I then added a timout to automatically remove the tag but then removed again as it does not make any sense to do that: if common.js is not loaded properly, the init of the page will fail anyway because S() is not called. Other than that I did not find any bugs related to this PR so I think its good to merge.

edit: tested with ESP8266 on Chrome, Safari, Firefox and Android. Also tested in AP mode in Chrome and Android. Not tested on iPhone and Opera.

DedeHai avatar Nov 16 '25 11:11 DedeHai

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: there is another issue: the ws-connection appears to be less stable. On a C3 with high FPS (>100) the connection breaks when using "peek" and clicking a button. Need to investigate.

DedeHai avatar Nov 16 '25 12:11 DedeHai

I was no longer able to reproduce the connection issue I saw on the C3, not sure what the issue was there, it was rock-solid in my recent tests even at 120FPS and in AP mode. The only real issue I am seeing is with cached pages: if I switch back and forth between versions and dont do a full page refresh, the UI can hang or act all weird.

DedeHai avatar Nov 22 '25 12:11 DedeHai

In the latest commit I removed the hiding of the "(un)freeze" button in segments. Does anyone know the reason why it was hidden if a segment is not collapsed?

DedeHai avatar Nov 22 '25 12:11 DedeHai

Why have you removed the Awaiting Testing label without comment from someone else's testing @DedeHai ?

netmindz avatar Nov 26 '25 20:11 netmindz

I added that as a reminder to myself as I did not deem it tested enough. I ran many tests on this on different platforms, in AP mode, on mobile, different browsers. I can add the label back in if anyone still wants to and is willing to run more tests.

DedeHai avatar Nov 26 '25 20:11 DedeHai

The only thing that I found that could cause trouble is browser cache: if the browser uses cached files from a previous version, the UI can break. I did not find a good way to enforce an automatic full reload so if there is an easy way to do that it should be added.

DedeHai avatar Nov 26 '25 21:11 DedeHai

I did not find a good way to enforce an automatic full reload so if there is an easy way to do that it should be added.

Today: change VERSION for each build -- it's used as the cache key. Later we can patch up cdata.js to also include static content hashes.

willmmiles avatar Nov 27 '25 22:11 willmmiles

I did not find a good way to enforce an automatic full reload so if there is an easy way to do that it should be added.

Today: change VERSION for each build -- it's used as the cache key. Later we can patch up cdata.js to also include static content hashes.

I guess later was today (#5120)! I'm still living in the past, I guess..

willmmiles avatar Nov 28 '25 02:11 willmmiles