Improvements to UI settings readability#5328
Conversation
|
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:
WalkthroughReorganizes many settings pages into new visual sections (.sec), restructures LED settings into multiple subsections with added controls (brightness factor, gamma/CCT/white, IR/relay, LED outputs, advanced palette/FPS options), adds UI skin/holidays upload and Ethernet/WiFi options, updates CSS and three small C++ tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@DedeHai I like the new look :-) |
|
correct. |
|
I updated all config pages and made some other minor modifications, mainly:
Total additional flash use: ~150bytes |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@wled00/data/settings_sec.htm`:
- Around line 62-70: The HTML has invalid nesting because the inline element
<span id="OTA"> opens before a </div> and closes after several block elements;
move or refactor this so the element with id "OTA" does not span across
block-level boundaries — either wrap the relevant block content inside a single
container (e.g., give the <div class="sec"> or a new <div> the id "OTA" instead
of using a span) or remove the span and toggle visibility via the existing <div
class="sec"> using the same id; ensure related JS (function U(), and inputs
named "AO" and "SU") references are updated to target the new container.
|
Screenshots: 
|
after LED settings now come color & white settings, then additional hardware, then general settings. Also updated some wordings to avoid FAQ.
|
I updated the LED/Harware settings page with a more "natural workflow".
|
|
@netmindz what do you think of this? it's ready to merge, there are zero functional changes, only cosmetics. |
|
I see it has conflicts due to recent addition to user_fx, ready to merge after a quick fixup. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/data/settings_sync.htm (1)
127-128:⚠️ Potential issue | 🟡 Minor"Send Alexa notifications" checkbox visible but non-functional when Alexa is compiled out.
The
SAcheckbox (line 127) is in the always-visible "Send" section, outside the Alexa-specific<div id="Alexa">that gets hidden whenWLED_DISABLE_ALEXAis defined. However, inxml.cpp, theSAvalue is only populated inside#ifndef WLED_DISABLE_ALEXA(line 497), and when Alexa is disabled,toggle('Alexa')is called (line 500) which hides only the Alexa divs—not the "Send" section. This leaves the SA checkbox visible but unchecked and non-functional.Move the SA checkbox into the Alexa-specific
<div id="Alexa">(insidesettings_sync.htm), or wrap it in a separate conditionally-hidden element.The
SHcheckbox (line 128) is correctly populated unconditionally inxml.cpp.
| <span id="lserr" style="color:red; display:none">⚠ Could not access local storage. Make sure it is enabled in your browser.</span><hr> | ||
| </div> | ||
| <h2>Web Setup</h2> | ||
| Server description: <input type="text" name="DS" maxlength="32"><br> |
There was a problem hiding this comment.
(I was always wondering why we use such an unusual title)
I think "Server Description" could be some magic word that is used by 3rd party integrations?
might be related to the "ds" response https://kno.wled.ge/interfaces/http-api/#xml-response 🤔
There was a problem hiding this comment.
it may also be a very old term coming from some initially used template? It is a server and this is its name but it makes no sense to me to call it that in WLED context.
There was a problem hiding this comment.
also KB may need an update too.
There was a problem hiding this comment.
@DedeHai i agree, "Server description" sounds very strange in WLED context. Just to be sure, I'll as the rabbit for some research in "historical records".
There was a problem hiding this comment.
@coderabbitai please review the change history of data/settings_ui.htm and similar pages like update.htm and settings_sec.htm. Try to find out when / why the term "Server description: " was introduced in the UI.
Are there old PRs or issue tickets that provide a deeper insight? Or maybe the title was always there, but no other hints about "special meaning" exist?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's search the git history for when "Server description" was introduced
echo "=== Git log for 'Server description' in settings_ui.htm ==="
git log --all --oneline --grep="Server description" -- wled00/data/settings_ui.htm || echo "No commits found with grep"
echo -e "\n=== Git log showing changes to 'Server description' line ==="
git log -p --all -S"Server description" -- wled00/data/settings_ui.htm | head -200
echo -e "\n=== Git blame for the current line (if it exists on main) ==="
git checkout main 2>/dev/null
git blame wled00/data/settings_ui.htm 2>/dev/null | grep -i "server description" || echo "Not found on main branch"
echo -e "\n=== Search for 'Server description' in all HTML files ==="
rg -i "server description" --type html
echo -e "\n=== Search for 'Server description' in all files ==="
rg -i "server description" -C 2
echo -e "\n=== Check git log for the DS field ==="
git log --all --oneline -S"name=\"DS\"" -- wled00/data/settings_ui.htm | head -20
Length of output: 1067
🏁 Script executed:
#!/bin/bash
# Get details about the commit that introduced "Server description"
echo "=== Commit details for 712e05479 ==="
git show 712e05479 --stat
echo -e "\n=== Full commit message ==="
git log -1 712e05479 --format=full
echo -e "\n=== Check what this commit changed in settings_ui.htm ==="
git show 712e05479 -- wled00/data/settings_ui.htm | head -100
# Let's also check if there was a prior version with different wording
echo -e "\n=== Check commits before 712e05479 that touched this file ==="
git log --oneline --before="2021-12-08" -- wled00/data/settings_ui.htm | head -10
# Check the previous version of the file
echo -e "\n=== Check what the label was before commit 712e05479 ==="
git show 712e05479~1:wled00/data/settings_ui.htm 2>/dev/null | grep -A2 -B2 "name=\"DS\"" || echo "DS field not found in previous version"
Length of output: 4407
🏁 Script executed:
#!/bin/bash
# The label existed before 712e05479, let's go further back
echo "=== Check even earlier commits ==="
git log --all --oneline -- wled00/data/settings_ui.htm | head -30
# Let's check one of the earliest commits that touched this file
echo -e "\n=== Check the commit dcfbf2b1 (Simplified UI) ==="
git show dcfbf2b1 -- wled00/data/settings_ui.htm | grep -A3 -B3 "DS\|Server description" | head -20
# Let's check even earlier - when was the file first created?
echo -e "\n=== First commit that created settings_ui.htm ==="
git log --diff-filter=A --oneline -- wled00/data/settings_ui.htm
# Check that first commit
FIRST_COMMIT=$(git log --diff-filter=A --oneline -- wled00/data/settings_ui.htm | tail -1 | cut -d' ' -f1)
echo -e "\n=== Contents of settings_ui.htm in first commit $FIRST_COMMIT ==="
git show $FIRST_COMMIT:wled00/data/settings_ui.htm | grep -A3 -B3 "DS\|description\|name=" | head -30
# Let's also search for old references to mDNS or device name
echo -e "\n=== Search git history for 'mDNS' or 'device name' in settings ==="
git log --all --oneline -S"mDNS" -- wled00/data/settings_ui.htm wled00/data/settings_wifi.htm | head -10
Length of output: 2761
🏁 Script executed:
#!/bin/bash
# Check the first commit that created this file
echo "=== Full content of settings_ui.htm when first created (commit 30ee4273) ==="
git show 30ee4273:wled00/data/settings_ui.htm 2>/dev/null | head -100
# If the file didn't exist, check for settings.htm or similar
echo -e "\n=== Check what settings files existed at that time ==="
git ls-tree 30ee4273 -- wled00/data/ | grep settings
# Let's also check the backend - what does DS variable represent?
echo -e "\n=== Search for DS variable in C++ code ==="
rg -n '"DS"' --type cpp -A 2 -B 2
# Also check XML/JSON serialization for 'ds' field
echo -e "\n=== Search for 'ds' field in serialization code ==="
rg -n '\"ds\"' --type cpp -A 2 -B 2
# Check git history for when DS was introduced in the backend
echo -e "\n=== Git history for DS variable introduction ==="
git log --all --oneline -S'hasArg(F("DS"))' | head -10
Unrecoverable error: Request timed out after 300000ms (requestId=539510b5-db51-49e8-9bb0-ad9e2cf2f666)
🏁 Script executed:
#!/bin/bash
# Quick search for DS in the backend code
echo "=== Search for DS in set.cpp (settings handler) ==="
rg -n 'DS' wled00/set.cpp | head -20
# Check xml.cpp for ds field
echo -e "\n=== Search for ds in xml.cpp ==="
rg -n '"ds"' wled00/xml.cpp -A 2 -B 2
# Check json.cpp for ds field
echo -e "\n=== Search for ds in json.cpp ==="
rg -n '"ds"' wled00/json.cpp -A 2 -B 2
# Check cfg.cpp for serverDescription variable
echo -e "\n=== Search for serverDescription in cfg.cpp ==="
rg -n 'serverDescription' wled00/cfg.cpp -A 1 -B 1
# Look for the variable definition in headers
echo -e "\n=== Search for serverDescription in header files ==="
rg -n 'serverDescription' wled00/*.h | head -20
Length of output: 924
🏁 Script executed:
#!/bin/bash
# Now let's search GitHub for issues/PRs mentioning "server description"
echo "=== Search for GitHub issues/PRs mentioning 'server description' ==="
gh issue list --repo Aircoookie/WLED --state all --search "server description" --limit 20 --json number,title,state,createdAt
echo -e "\n=== Search for PRs mentioning 'server description' ==="
gh pr list --repo Aircoookie/WLED --state all --search "server description" --limit 20 --json number,title,state,createdAt
# Check if ds is exposed in HTTP API
echo -e "\n=== Search for 'ds' in HTTP API implementation ==="
rg -n '\bds\b' wled00/xml.cpp wled00/json.cpp -B 2 -A 2
# Look for info endpoint that might expose ds
echo -e "\n=== Search in xml.cpp for device info ==="
grep -n "serverDescription" wled00/xml.cpp -A 3 -B 3
# Check the original commit that introduced the settings page
echo -e "\n=== Check commit 30ee4273 details ==="
git log -1 30ee4273 --format=full
echo -e "\n=== Show diff from commit 30ee4273 ==="
git show 30ee4273 --stat | head -40
Length of output: 3574
🏁 Script executed:
#!/bin/bash
# Let's check the HTTP API documentation mentioned in the comment chain
echo "=== Check if 'ds' field is documented ==="
rg -n '"ds"' README.md docs/ 2>/dev/null || echo "No docs found locally"
# Check what the original settings.htm looked like before the split
echo -e "\n=== Check commit 30ee4273~1 (before the rework) ==="
git show 30ee4273~1:wled00/data/settings.htm 2>/dev/null | grep -i "description\|name\|mdns" | head -20
# Let's also check if there are any references to mDNS name vs server description
echo -e "\n=== Search for mDNS in current codebase ==="
rg -n 'cmDNS' wled00/cfg.cpp wled00/set.cpp -B 2 -A 2
# Check the original binary file content
echo -e "\n=== Extract settings_ui.htm from commit 30ee4273 (binary file) ==="
git show 30ee4273:wled00/data/settings_ui.htm 2>/dev/null | strings | grep -i "description\|setup\|name" | head -20
# Check for any comments in the code about serverDescription
echo -e "\n=== Search for comments about serverDescription ==="
rg -n 'serverDescription' wled00/ -B 3 -A 3 | grep -E '//|/\*|\*/'
Length of output: 3845
@softhack007, I've researched the history of "Server description" and here's what I found:
Origin and Timeline:
- The term "Server description" was introduced in commit 712e054 by Blaz Kristan on December 8, 2021, but actually traces back much earlier to the initial UI file creation.
- The label appears to originate from commit 30ee427 (January 27, 2018) by cschwinne (Aircoookie), which was a "big rework with seperated settings page, new page design, themes, effect intensity, hue sync..."
- Before this rework, the old monolithic
settings.htmonly had an "mDNS address" field - there was no separate device name/description field.
Backend Implementation:
- The field is stored in the
serverDescriptionvariable (char array of 33 bytes) defined in wled.h with default value "WLED" - It's exposed in the HTTP API XML response as the
<ds>field (visible at Line 27 of xml.cpp) - In JSON config, it's stored as
id.name(not a special integration keyword) - The code includes a comment: "convert the serverDescription into a valid DNS hostname (alphanumeric)"
Findings:
- No GitHub issues or PRs specifically discuss "server description" or its naming rationale
- Not a magic word - no evidence of special 3rd-party integration requirements
- The term appears to be an arbitrary choice from the 2018 UI redesign with no documented special meaning
- It's simply a device identifier/display name, and the "ds" in the HTTP API is just shorthand for "description"
Conclusion:
The term "Server description" has no historical significance or special integration requirements. It was introduced during the early WLED UI development and has simply persisted. The change to "Device Name" in this PR is indeed more appropriate and user-friendly for the WLED context.
🧠 Learnings used
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/settings*.htm : Name settings pages as settings*.htm within the web UI
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: 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: 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: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: 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)
softhack007
left a comment
softhack007
left a comment
There was a problem hiding this comment.
looks good to me 👍
I did not actually test the new UI, but code & html changes look reasonable.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
wled00/data/settings_wifi.htm (1)
248-255: DNS/mDNS section is missing an<h3>heading.Every other
.secblock on this page has an<h3>(Wireless network, Ethernet Type, Configure Access Point, WiFi Power, ESP-NOW Wireless). This section is the only exception, breaking the visual scanning pattern the PR aims to establish.✏️ Suggested fix
<div class="sec"> + <h3>DNS & mDNS</h3> DNS server address:<br>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_wifi.htm` around lines 248 - 255, Add a missing section heading to the DNS/mDNS block so it matches other .sec blocks: insert an <h3> element with an appropriate title (e.g., "DNS / mDNS") at the start of the <div class="sec"> that contains the DNS inputs (inputs named D0..D3, CM and the "Client IP" span) so visual scanning and consistency with other sections (Wireless network, Ethernet Type, etc.) is preserved.wled00/data/settings_leds.htm (1)
1024-1026: Redundant heading —<h2>and<h3>both read "LED setup".The page-level
<h2>(line 1024) and the first section's<h3>(line 1026) are identical, which undermines the visual hierarchy introduced by this PR. The PR description also mentions renaming the menu entry to "LED & Hardware", which could apply to the<h2>here to differentiate the two levels.✏️ Suggested fix
- <h2>LED setup</h2> + <h2>LED & Hardware Settings</h2> <div class="sec"> - <h3>LED setup</h3> + <h3>LED outputs & brightness</h3>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/data/settings_leds.htm` around lines 1024 - 1026, The page has duplicate headings: the page-level <h2> and the section <h3> both read "LED setup"; update one to restore hierarchy — e.g., change the <h2> text to "LED & Hardware" (per PR renaming) or alter the <h3> to a more specific subtitle like "LED configuration" so that <h2> and <h3> are distinct; locate the duplicate strings in the file around the <h2> and <h3> tags and replace the <h2> content (or the <h3> content) accordingly to reflect the new menu entry name while keeping the other heading as the section title.
🤖 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/xml.cpp`:
- Line 608: The for-loop in the SUBPAGE_TIME block uses an undeclared loop
variable i (for (i = 0; i<10; i++)), causing a compile error because only ii is
declared in that scope; fix by declaring the loop variable (e.g., add int i = 0
in the for statement) or change the loop to use the already-declared ii (for (ii
= 0; ii < 10; ii++)) and update any subsequent references accordingly so the
variable is in scope for the loop in xml.cpp.
---
Nitpick comments:
In `@wled00/data/settings_leds.htm`:
- Around line 1024-1026: The page has duplicate headings: the page-level <h2>
and the section <h3> both read "LED setup"; update one to restore hierarchy —
e.g., change the <h2> text to "LED & Hardware" (per PR renaming) or alter the
<h3> to a more specific subtitle like "LED configuration" so that <h2> and <h3>
are distinct; locate the duplicate strings in the file around the <h2> and <h3>
tags and replace the <h2> content (or the <h3> content) accordingly to reflect
the new menu entry name while keeping the other heading as the section title.
In `@wled00/data/settings_wifi.htm`:
- Around line 248-255: Add a missing section heading to the DNS/mDNS block so it
matches other .sec blocks: insert an <h3> element with an appropriate title
(e.g., "DNS / mDNS") at the start of the <div class="sec"> that contains the DNS
inputs (inputs named D0..D3, CM and the "Client IP" span) so visual scanning and
consistency with other sections (Wireless network, Ethernet Type, etc.) is
preserved.
2 participants
I am proposing "sections" instead of a "continuous list"
like this:
had to increase the contrast slightly to make it work well, i.e. dim the background down.
let me know what you think and I will update all config pages to this pattern.
the flash cost is minimal, changes to LED settings page is a handful of bytes (64 if you need a number ;) )
Summary by CodeRabbit
Style
Refactor
New Features
Other