OTA update page restyling, automatically set download URL based on info.repo#5419
OTA update page restyling, automatically set download URL based on info.repo#5419softhack007 merged 15 commits intomainfrom
Conversation
* set "download latest binary" URL and badge based on info.repo * directly link to "latest" * use same style as other settings pages * correct bad name of "Security & Updates" page
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenames Security page title, restructures the Update UI/DOM (top row, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
CONFIG_IDF_TARGET_ESP32 is only defined on classic esp32 (pico boards included)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/data/update.htm (1)
27-33:⚠️ Potential issue | 🟡 MinorKeep the release link hidden until a valid repo is confirmed.
The hard-coded upstream badge/link still render before
/json/inforesolves, and they remain visible in the.catch()path. On custom or offline builds that can still send users to the wrong firmware. Default this block to hidden and only unhide it after a validdata.repois loaded.Proposed fix
- if (data.repo && data.repo !== "unknown") { + const releaseDownload = gId('release-download'); + if (data.repo && data.repo !== "unknown") { document.querySelector('.release-repo').href = "https://github.com/" + data.repo + "/releases/latest"; document.querySelector('.release-badge').src = "https://img.shields.io/github/release/" + data.repo + ".svg?style=flat-square"; + releaseDownload.classList.remove("hide"); } else { - gId('release-download').classList.add("hide"); + releaseDownload.classList.add("hide"); } ... .catch(error => { console.log('Could not fetch device info:', error); // Fallback to compiled-in value if API call fails document.querySelector('.installed-version').textContent = 'Unknown'; document.querySelector('.release-name').textContent = 'Unknown'; + gId('release-download').classList.add("hide"); }); ... - <div id="release-download"> + <div id="release-download" class="hide">Also applies to: 46-50, 74-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/update.htm` around lines 27 - 33, Hide the release link/badge by default and only unhide them when a valid repository is confirmed: add the "hide" class (or set display:none) to the elements targeted by document.querySelector('.release-repo'), document.querySelector('.release-badge') and gId('release-download') before the /json/info fetch, then inside the success branch where you check data.repo && data.repo !== "unknown" remove the "hide" class and set the href/src values (the block that references data.repo, '.release-repo', '.release-badge', and gId('release-download')); ensure the catch/error path leaves them hidden so custom/offline builds never expose incorrect links.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/data/update.htm`:
- Around line 71-79: The markup nests a block-level element (`#release-download`
div) inside a paragraph causing implicit paragraph closure; fix by removing the
<div id="release-download"> from inside the <p> (e.g., close the paragraph
before the `#release-download` block or convert the wrapping <p> to a block-level
container) so the "Installed version" and "Release" spans (classes sip
installed-version and sip release-name) remain in their own paragraph and the
release-download block is a separate sibling element.
---
Duplicate comments:
In `@wled00/data/update.htm`:
- Around line 27-33: Hide the release link/badge by default and only unhide them
when a valid repository is confirmed: add the "hide" class (or set display:none)
to the elements targeted by document.querySelector('.release-repo'),
document.querySelector('.release-badge') and gId('release-download') before the
/json/info fetch, then inside the success branch where you check data.repo &&
data.repo !== "unknown" remove the "hide" class and set the href/src values (the
block that references data.repo, '.release-repo', '.release-badge', and
gId('release-download')); ensure the catch/error path leaves them hidden so
custom/offline builds never expose incorrect links.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 24e4456e-7a7b-47c7-8887-b7b7702200f7
📒 Files selected for processing (2)
wled00/data/update.htmwled00/json.cpp
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
? is not a useful accessible name. Adding aria-label="Help" (and ideally title) makes the new icon-only control easier to discover.
|
Just noticed: it looks like the second "back" button should go on the bottom of the page instead of somewhere in between, as it is on other config pages. Also drop the "setup" in the "security & update setup" title(s)? or is there anything to "set up" on that page? |
|
Hello, 
it looks kind of odd seeing the Bootloader Update part here. It would be nice if on a normal software update the bootloader update part would be hidden since the first time i saw this i thought i made a mistake and i had clicked on updating the bootloader |
@DedeHai can you check how it looks on 8266? I only have esp32 boards, so I can't test 8266. I would expect that only the "normal" update shows on 8266, because all firmware images for 8266 are "factory image" = bootloader + wled in one file. |
good points 👍 i'll check |
i have this on all my esp32 (several variants) with a version form 11.02.26 , i didnt check the newest changes of the last 4 weeks. |
* move "WLED Software update" heading into the box * removed toast magic hook * move "back" button to bottom of the page, and hide it when an update is ongoing * some space between boxes * remaned "update!" to "update WLED!" (to avoid user confusion)
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Yes, in the first section users can setup OTA lock and password. |
* use getURL in fetch * remove additional div in bootloader section
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
* initially show a placeholder without badge * un-hide the download link after receiving info.repo * completely hide the download line when repo is unknown
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai please verity if the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5466 🏁 Script executed: Length of output: 840
✅ Browser / WebView Compatibility — No Blocking IssuesAll JavaScript features used compile cleanly against modern runtimes:
|
adding "white-space:pre-wrap" to ensure that \n is handled properly by all frontends.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Tested and behaves as expected on my boards -> ready to merge. |
|
@coderabbitai re-review all |
🧩 Analysis chain🏁 Script executed: Length of output: 9300
|
| Item | Status |
|---|---|
fetch(getURL('/json/info')) URL helper |
✅ Correct |
toggle('release-download') show/hide via common.js convention |
✅ Toggles both #release-download and #Norelease-download correctly |
gId('Norelease-download').classList.add("hide") on invalid repo / fetch failure |
✅ Hides "Checking..." placeholder cleanly |
allowBl check with .toLowerCase() for arch comparison |
✅ Handles ESP.getChipModel() casing variations |
data.arch == "esp8266" (case-sensitive) |
✅ json.cpp line 831 consistently returns lowercase "esp8266" |
bootloader-hash — white-space: pre-wrap + innerText with \n |
✅ Addressed correctly; wrapping works in all frontends including WebView |
data.repo used only via .href/.src assignment, not innerHTML |
✅ No XSS risk |
toggle('bootupd') when no #Nobootupd exists |
✅ toggle() guards with if (n), safe |
KrX3D's bootloader-section-during-update concern — Addressed ✅
hideforms() now explicitly hides #bootupd:
function hideforms() {
gId('toprow').classList.add("hide");
gId('backbtn').classList.add("hide");
gId('bootupd').classList.add("hide"); // ← hides bootloader section
toggle('upd');
}Triggered by onsubmit on both the software update form and the bootloader update form, so the sibling section is always hidden during any update.
DedeHai's back button placement — Addressed ✅
#backbtn is the last element in <body>, correctly at the bottom of the page. Both back buttons are hidden during an update via hideforms().
Previously-flagged minor issues — still present (non-blocking):
window.open(getURL("/update?revert"), "_self")— some restricted WKWebView configurations can interceptwindow.open().window.location.href = getURL("/update?revert")is the idiomatic same-document navigation.<meta content='width=device-width' name='viewport'>— missinginitial-scale=1; inconsistent withsettings_sec.htmwhich has the full viewport meta.type="text/javascript"on the<script>tag — redundant in HTML5.onchange="sV.value=checked?1:''"— uses implicit named-element global (sV) andwith-scopechecked;document.getElementById('sV').value=this.checked?1:''is unambiguous.
wled00/data/settings_sec.htm
Title changed to "Security & Update Setup" — confirmed appropriate by @softhack007 since the page contains OTA lock and password configuration. ✅
wled00/json.cpp
`#if` defined(ARDUINO_ARCH_ESP32) && defined(CONFIG_IDF_TARGET_ESP32)
root[F("arch")] = "esp32"; // hardcoded lowercase for classic ESP32
`#else`
root[F("arch")] = ESP.getChipModel(); // S2/S3/C3 etc.
`#endif`This ensures classic ESP32 reports the same lowercase casing convention as "esp8266", which is the correct direction. The JS allowBl check already uses .toLowerCase() so this doesn't change runtime behavior, but it makes the API contract consistent. ✅
Summary: The PR is well-structured, the two main UX concerns (bootloader section visibility during update, back button placement) are correctly addressed, and the dynamic release URL/badge logic is sound. The four minor items above are low-priority and don't block a merge.
🧠 Learnings used
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.
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.
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.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-11-14T13:37:11.994Z
Learning: In WLED code reviews, file operations (especially file.open()) should be checked to ensure they respect LittleFS filename limitations. The default LittleFS filename limit is 255 bytes (LFS_NAME_MAX). Reviews should assume default WLED configuration defines and not extreme edge-case values (e.g., WLED_MAX_SEGNAME_LEN = 512 would not be standard). File paths should be validated to stay within the 255-byte limit.
Learnt from: DedeHai
Repo: wled/WLED PR: 5404
File: wled00/data/pixelforge/pixelforge.htm:633-641
Timestamp: 2026-03-05T15:54:44.117Z
Learning: In the WLED codebase, `common.js` (wled00/data/common.js) is the shared utility file loaded by all web UI pages. It already contains helpers like gId, cE, getURL, showToast, etc. It is the appropriate place to add shared sanitization helpers (esc, safeUrl) since they should be available to all pages without duplication. The esc/safeUrl helpers are only strictly necessary for pixelforge.htm's renderTools() because that is the only page fetching content from an external remote source (GitHub Pages). All other innerHTML+template-literal injection points in the WLED web UI (index.js, settings_time.htm, settings_leds.htm, cpal/cpal.htm) use device-local JSON which is considered trusted in the WLED threat model.
Learnt from: softhack007
Repo: wled/WLED PR: 5381
File: wled00/data/update.htm:2-2
Timestamp: 2026-02-19T13:24:28.983Z
Learning: In WLED, generated embedded header files (html_*.h, js_iro.h, etc.) are NOT stored in the repository and have not been for over 2 years. These files are automatically regenerated during the build process from the source files in wled00/data/. Only the source .htm, .html, .css, and .js files in wled00/data/ should be committed when making web UI changes.
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.
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: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
Learnt from: netmindz
Repo: wled/WLED PR: 5093
File: wled00/util.cpp:1159-1182
Timestamp: 2025-11-20T00:04:04.829Z
Learning: In WLED PR `#5093`, the deviceId feature is designed for opt-in usage reporting that tracks only version/upgrade information (non-behavioral data), not user activity patterns. The deterministic salt approach (MAC + "WLED" + chip model/revision) is acceptable for this limited use case, as correlating MAC addresses to version history represents minimal privacy risk compared to behavioral tracking.
Learnt from: willmmiles
Repo: wled/WLED PR: 4998
File: wled00/ota_update.cpp:120-124
Timestamp: 2025-10-11T17:56:24.923Z
Learning: In the WLED OTA update implementation (wled00/ota_update.cpp), the parameter name for bypassing OTA validation is "skipValidation", not "ignoreRelease". The intent evolved from the original spec in issue `#4929` to disable all validation checks, not just release name checks, which is why the broader parameter name "skipValidation" is used.
✅ Actions performed
Full review triggered.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
5 participants
old:
--> new:
Summary by CodeRabbit
New Features
Improvements
Behavior
Improvements