Add report version feature #5116
Conversation
WalkthroughUncomments a button margin in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
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 (1)
wled00/data/index.js (1)
3360-3416: Implementation looks good, but consider CSS consistency.The dialog implementation correctly handles all three user choices. The privacy policy link is a good practice for transparency.
Minor observation: The extensive inline styles (line 3368-3370) could be moved to
index.cssfor consistency with the existing.overlayanddialogstyles, but this is acceptable as-is for an isolated feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/data/index.css(1 hunks)wled00/data/index.js(1 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.csswled00/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.csswled00/data/index.js
🧠 Learnings (5)
📓 Common learnings
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: 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, 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.
📚 Learning: 2025-11-20T00:04:04.829Z
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.
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-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/index.js
🧬 Code graph analysis (1)
wled00/data/index.js (1)
tools/cdata.js (1)
version(94-94)
🔇 Additional comments (3)
wled00/data/index.css (1)
795-798: LGTM!Enabling button margins supports the new upgrade prompt dialog UI. The styling change is minimal and appropriate.
wled00/data/index.js (2)
3307-3309: Summary: Good feature implementation with one critical gap.The version reporting feature is well-structured with proper:
- Opt-in consent flow with clear user choices
- Privacy policy link for transparency
- Error handling that doesn't lock users out on failure
- AP mode detection to suppress when no internet is available
Based on learnings, this opt-in approach for non-behavioral version/upgrade data is appropriate for privacy.
The critical issue to address is adding the call to
checkVersionUpgrade(info)at an appropriate initialization point.
3472-3494: No file path inconsistency issue exists — the/uploadendpoint automatically normalizes filenames.The
/uploadendpoint'shandleUpload()function (wled_server.cpp:194-195) automatically prepends a leading slash if missing:if (finalname.charAt(0) != '/') { finalname = '/' + finalname; }Both
'version-info.json'(line 3481) and/version-info.json(line 3319) resolve to the same file location in the root filesystem. The stylistic inconsistency with/presets.json(which uses a leading slash) doesn't affect functionality since the server normalizes the path either way.
|
So the current wording is
What about
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(2 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 (5)
📓 Common learnings
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.
📚 Learning: 2025-11-20T00:04:04.829Z
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.
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-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-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/index.js
🪛 GitHub Check: CodeQL
wled00/data/index.js
[failure] 3385-3397: Client-side cross-site scripting
Cross-site scripting vulnerability due to user-provided value.
⏰ 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). (19)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32S3_wroom2)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (1)
wled00/data/index.js (1)
696-697: Hooking version check intoparseInfolooks goodCalling
checkVersionUpgrade(i)fromparseInfo()ensures you have the full/json/infopayload available and, combined with theversionCheckDoneguard, avoids repeated prompts while still reacting to upgrades. No functional issues spotted here.
| function checkVersionUpgrade(info) { | ||
| // Only check once per page load | ||
| if (versionCheckDone) return; | ||
| versionCheckDone = true; | ||
|
|
||
| // Suppress feature if in AP mode (no internet connection available) | ||
| if (info.wifi && info.wifi.ap) return; | ||
|
|
||
| // Fetch version-info.json using existing /edit endpoint | ||
| fetch(getURL('/edit?func=edit&path=/version-info.json'), { | ||
| method: 'get' | ||
| }) | ||
| .then(res => { | ||
| if (res.status === 404) { | ||
| // File doesn't exist - first install, show install prompt | ||
| showVersionUpgradePrompt(info, null, info.ver); | ||
| return null; | ||
| } | ||
| if (!res.ok) { | ||
| throw new Error('Failed to fetch version-info.json'); | ||
| } | ||
| return res.json(); | ||
| }) | ||
| .then(versionInfo => { | ||
| if (!versionInfo) return; // 404 case already handled | ||
|
|
||
| // Check if user opted out | ||
| if (versionInfo.neverAsk) return; | ||
|
|
||
| // Check if version has changed | ||
| const currentVersion = info.ver; | ||
| const storedVersion = versionInfo.version || ''; | ||
|
|
||
| if (storedVersion && storedVersion !== currentVersion) { | ||
| // Version has changed, show upgrade prompt | ||
| showVersionUpgradePrompt(info, storedVersion, currentVersion); | ||
| } else if (!storedVersion) { | ||
| // Empty version in file, show install prompt | ||
| showVersionUpgradePrompt(info, null, currentVersion); | ||
| } | ||
| }) | ||
| .catch(e => { | ||
| console.log('Failed to load version-info.json', e); | ||
| // On error, save current version for next time | ||
| if (info && info.ver) { | ||
| updateVersionInfo(info.ver, false); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| function showVersionUpgradePrompt(info, oldVersion, newVersion) { | ||
| // Determine if this is an install or upgrade | ||
| const isInstall = !oldVersion; | ||
|
|
||
| // Create overlay and dialog | ||
| const overlay = d.createElement('div'); | ||
| overlay.id = 'versionUpgradeOverlay'; | ||
| overlay.style.cssText = 'position:fixed;top:0;left:0;width:100%;height:100%;background:rgba(0,0,0,0.7);z-index:10000;display:flex;align-items:center;justify-content:center;'; | ||
|
|
||
| const dialog = d.createElement('div'); | ||
| dialog.style.cssText = 'background:var(--c-1);border-radius:10px;padding:25px;max-width:500px;margin:20px;box-shadow:0 4px 6px rgba(0,0,0,0.3);'; | ||
|
|
||
| // Build contextual message based on install vs upgrade | ||
| const title = isInstall | ||
| ? '🎉 Thank you for installing WLED!' | ||
| : '🎉 WLED Upgrade Detected!'; | ||
|
|
||
| const description = isInstall | ||
| ? `You are now running WLED <strong style="text-wrap: nowrap">${newVersion}</strong>.` | ||
| : `Your WLED has been upgraded from <strong style="text-wrap: nowrap">${oldVersion}</strong> to <strong style="text-wrap: nowrap">${newVersion}</strong>.`; | ||
|
|
||
| const question = 'Would you like to help the WLED development team by reporting your installation? This helps us understand what hardware and versions are being used.' | ||
|
|
||
| dialog.innerHTML = ` | ||
| <h2 style="margin-top:0;color:var(--c-f);">${title}</h2> | ||
| <p style="color:var(--c-f);">${description}</p> | ||
| <p style="color:var(--c-f);">${question}</p> | ||
| <p style="color:var(--c-f);font-size:0.9em;"> | ||
| <a href="https://kno.wled.ge/about/privacy-policy/" target="_blank" style="color:var(--c-6);">Learn more about what data is collected and why</a> | ||
| </p> | ||
| <div style="margin-top:20px;"> | ||
| <button id="versionReportYes" class="btn">Yes</button> | ||
| <button id="versionReportNo" class="btn">Not Now</button> | ||
| <button id="versionReportNever" class="btn">Never Ask</button> | ||
| </div> | ||
| `; | ||
|
|
||
| overlay.appendChild(dialog); | ||
| d.body.appendChild(overlay); | ||
|
|
||
| // Add event listeners | ||
| gId('versionReportYes').addEventListener('click', () => { | ||
| reportUpgradeEvent(info, oldVersion); | ||
| d.body.removeChild(overlay); | ||
| }); | ||
|
|
||
| gId('versionReportNo').addEventListener('click', () => { | ||
| // Don't update version, will ask again on next load | ||
| d.body.removeChild(overlay); | ||
| }); | ||
|
|
||
| gId('versionReportNever').addEventListener('click', () => { | ||
| updateVersionInfo(newVersion, true); | ||
| d.body.removeChild(overlay); | ||
| showToast('You will not be asked again.'); | ||
| }); | ||
| } | ||
|
|
||
| function reportUpgradeEvent(info, oldVersion) { | ||
| showToast('Reporting upgrade...'); | ||
|
|
||
| // Fetch fresh data from /json/info endpoint as requested | ||
| fetch(getURL('/json/info'), { | ||
| method: 'get' | ||
| }) | ||
| .then(res => res.json()) | ||
| .then(infoData => { | ||
| // Map to UpgradeEventRequest structure per OpenAPI spec | ||
| // Required fields: deviceId, version, previousVersion, releaseName, chip, ledCount, isMatrix, bootloaderSHA256 | ||
| const upgradeData = { | ||
| deviceId: infoData.deviceId, // Use anonymous unique device ID | ||
| version: infoData.ver || '', // Current version string | ||
| previousVersion: oldVersion || '', // Previous version from version-info.json | ||
| releaseName: infoData.release || '', // Release name (e.g., "WLED 0.15.0") | ||
| chip: infoData.arch || '', // Chip architecture (esp32, esp8266, etc) | ||
| ledCount: infoData.leds ? infoData.leds.count : 0, // Number of LEDs | ||
| isMatrix: !!(infoData.leds && infoData.leds.matrix), // Whether it's a 2D matrix setup | ||
| bootloaderSHA256: infoData.bootloaderSHA256 || '', // Bootloader SHA256 hash | ||
| brand: infoData.brand, // Device brand (always present) | ||
| product: infoData.product, // Product name (always present) | ||
| flashSize: infoData.flash // Flash size (always present) | ||
| }; | ||
|
|
||
| // Add optional fields if available | ||
| if (infoData.psram !== undefined) upgradeData.psramSize = infoData.psram; | ||
| // Note: partitionSizes not currently available in /json/info endpoint | ||
|
|
||
| // Make AJAX call to postUpgradeEvent API | ||
| return fetch('https://usage.wled.me/api/usage/upgrade', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify(upgradeData) | ||
| }); | ||
| }) | ||
| .then(res => { | ||
| if (res.ok) { | ||
| showToast('Thank you for reporting!'); | ||
| updateVersionInfo(info.ver, false); | ||
| } else { | ||
| showToast('Report failed. Please try again later.', true); | ||
| // Do NOT update version info on failure - user will be prompted again | ||
| } | ||
| }) | ||
| .catch(e => { | ||
| console.log('Failed to report upgrade', e); | ||
| showToast('Report failed. Please try again later.', true); | ||
| // Do NOT update version info on error - user will be prompted again | ||
| }); | ||
| } | ||
|
|
||
| function updateVersionInfo(version, neverAsk) { | ||
| const versionInfo = { | ||
| version: version, | ||
| neverAsk: neverAsk | ||
| }; | ||
|
|
||
| // Create a Blob with JSON content and use /upload endpoint | ||
| const blob = new Blob([JSON.stringify(versionInfo)], { type: 'application/json' }); | ||
| const formData = new FormData(); | ||
| formData.append('data', blob, 'version-info.json'); | ||
|
|
||
| fetch(getURL('/upload'), { | ||
| method: 'POST', | ||
| body: formData | ||
| }) | ||
| .then(res => res.text()) | ||
| .then(data => { | ||
| console.log('Version info updated', data); | ||
| }) | ||
| .catch(e => { | ||
| console.log('Failed to update version-info.json', e); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Version reporting flow is coherent; update prompt wording and consider minor cleanups
The end‑to‑end flow — detecting install/upgrade via version-info.json, prompting once per version, posting only hardware/version metadata to https://usage.wled.me/api/usage/upgrade, and persisting version-info.json via /upload — looks logically sound and matches the “opt‑in, non‑behavioral usage reporting” intent from earlier work. Based on learnings, the fields you send are all device/firmware level, not behavioral.
A few focused points to address or consider:
-
Prompt text doesn’t yet match the PR’s privacy‑clarified wording
The current
questionstring:const question = 'Would you like to help the WLED development team by reporting your installation? This helps us understand what hardware and versions are being used.'
doesn’t incorporate the “one time action” and “no personal or behavioral data” clarification mentioned in the PR description. To align both with the PR and with what
reportUpgradeEventactually sends, I’d update it along these lines:
- const question = 'Would you like to help the WLED development team by reporting your installation? This helps us understand what hardware and versions are being used.'
- const question = 'Would you like to help the WLED development team by reporting your installation? This is a one time action that sends details of your hardware configuration, but no personal or behavioral data, e.g which esp32 chip variant, how many LED pixels etc';
2. **`reportUpgradeEvent` doesn’t really need the `info` parameter**
Inside `reportUpgradeEvent(info, oldVersion)` you immediately fetch fresh data from `/json/info` and build `upgradeData` from `infoData`. The only remaining use of `info` is:
```js
updateVersionInfo(info.ver, false);
To simplify the API and avoid dependence on the outer info object, you could rely purely on infoData:
- function reportUpgradeEvent(info, oldVersion) {
+ function reportUpgradeEvent(oldVersion) {
…
- .then(infoData => {
+ .then(infoData => {
…
- .then(res => {
+ .then(res => {
if (res.ok) {
showToast('Thank you for reporting!');
- updateVersionInfo(info.ver, false);
+ updateVersionInfo(infoData.ver || '', false);
…and at the call site:
- reportUpgradeEvent(info, oldVersion);
+ reportUpgradeEvent(oldVersion);This keeps the function self‑contained and matches the previous CodeQL feedback about an unnecessary info parameter.
-
InnerHTML/XSS and the CodeQL warning
dialog.innerHTML = \…${title}…${description}…${question}…`;is what CodeQL is flagging. In practice here the only interpolated values are firmware‑controlled strings (oldVersion/newVersion) plus a constantquestion, so this doesn’t introduce a new, attacker‑controlled XSS surface beyond existinginnerHTML` uses in this file.If you want to harden further or quiet CodeQL, an incremental improvement would be to construct the dialog content with DOM APIs and set dynamic pieces via
textContentinstead of string interpolation, while keeping the link/button structure the same. Given the current data sources, I’d treat that as a future security‑hardening refactor rather than a blocker for this PR. -
Process reminder for web UI changes
Since
wled00/data/index.jsis modified, remember to runnpm run buildbefore producing firmware binaries so that the embedded web assets stay in sync with the updated JS.
🧰 Tools
🪛 GitHub Check: CodeQL
[failure] 3385-3397: Client-side cross-site scripting
Cross-site scripting vulnerability due to user-provided value.
There was a problem hiding this comment.
Please update your comment to reflect the current code @coderabbitai
There was a problem hiding this comment.
@netmindz, I've reviewed the current code in the diff and my observations remain accurate. Here's an updated assessment:
Current State Analysis
The version reporting implementation is functionally sound, but the prompt wording still doesn't match your proposed privacy-clarified text from the PR description.
1. Prompt Text Needs Update (Line 3383)
Current code:
const question = 'Would you like to help the WLED development team by reporting your installation? This helps us understand what hardware and versions are being used.'Should be (per PR description):
const question = 'Would you like to help the WLED development team by reporting your installation? This is a one time action that sends details of your hardware configuration, but no personal or behavioral data, e.g which esp32 chip variant, how many LED pixels etc';This aligns the UI text with what's actually being collected and matches the privacy transparency discussed in PR #5093.
2. Redundant Parameter (Lines 3419-3455)
The reportUpgradeEvent(info, oldVersion) function receives info but only uses info.ver at the end (line 3455). Since it fetches fresh data from /json/info anyway, you could simplify to:
function reportUpgradeEvent(oldVersion) {
// ... fetch infoData ...
.then(res => {
if (res.ok) {
showToast('Thank you for reporting!');
updateVersionInfo(infoData.ver || '', false); // use infoData instead
}
// ...
}And update the call site (line 3403):
reportUpgradeEvent(oldVersion);3. CodeQL XSS Warning (Lines 3385-3397)
The dialog.innerHTML with template literals triggers a CodeQL warning. While not immediately exploitable (values come from firmware version strings), hardening with DOM APIs would be more secure. This is not a blocker but a future improvement opportunity.
4. Build Process
Remember to run npm run build after JS changes to update the embedded web assets.
The core logic—detecting install/upgrade, one-time prompting, hardware-only reporting, and persistence—is solid and aligns with the opt-in usage reporting design.
🧠 Learnings used
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: 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.
or the shorter
|
|
I like shorter version. Does this address your concerns @DedeHai ? |
|
I also like the sort version. Did feed it to an AI to get a few variants, this sounded best to me: in the policy, the last sentence is a bit confusing: |
| .btn { | ||
| padding: 8px; | ||
| /*margin: 10px 4px;*/ | ||
| margin: 10px 4px; |
There was a problem hiding this comment.
are you sure this wont break anything else in the UI?
There was a problem hiding this comment.
Buttons for a touch UI should always have a margin as otherwise you risk clicking the wrong item.
I'll have a check through however for other used of that class to see if this change is an improvement elsewhere too however
There was a problem hiding this comment.
I did not see anything obvious. just wondering why it was commented. Mayb just to save a few bytes.
|
Tested this and works as intended but it suffers from the same issue I posted in the last comment in #5013 - it wont show after an update unless I do a full refresh, at least in chrome. |
We might need to check what cache control related headers are being send and confirm the backend supports Not Modified responses. Please open a separate issue for that |
I like that, but I still want to get some kind of reference to the fact that it excludes anything behavioural/activity based
Yeah I agree that "automatically" would be better than "by default" |
It doesn't seem to me like |
Then why not: Help make WLED better with a one-time hardware report? It includes only device details like chip type, LED count, etc. — never personal data or your activities. About the policy, I think we can just do "No continuous telemetry is sent." |
It's on my list to kill this field at some point, a value that developers randomly update by hand is not acceptable. If we need it all, it should be updated by a pipeline that runs after a pr merge, this should never be something a developer is editing by hand on a branch |
|
Agreed. It has also caused merge conflicts in the past. I believe this value is very useful, updating it automatically after every push to main would make things a lot easier. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/data/index.js (1)
3420-3472: Remove unusedinfoparameter that was flagged in previous review.The
infoparameter is passed toreportUpgradeEventbut immediately replaced by fresh data from/json/info. The only usage is at Line 3461 (info.ver), which should useinfoData.verinstead.Update the function signature and the single usage:
-function reportUpgradeEvent(info, oldVersion) { +function reportUpgradeEvent(oldVersion) { showToast('Reporting upgrade...'); // Fetch fresh data from /json/info endpoint as requested fetch(getURL('/json/info'), { method: 'get' }) .then(res => res.json()) .then(infoData => { // Map to UpgradeEventRequest structure per OpenAPI spec // Required fields: deviceId, version, previousVersion, releaseName, chip, ledCount, isMatrix, bootloaderSHA256 const upgradeData = { deviceId: infoData.deviceId, // Use anonymous unique device ID version: infoData.ver || '', // Current version string previousVersion: oldVersion || '', // Previous version from version-info.json releaseName: infoData.release || '', // Release name (e.g., "WLED 0.15.0") chip: infoData.arch || '', // Chip architecture (esp32, esp8266, etc) ledCount: infoData.leds ? infoData.leds.count : 0, // Number of LEDs isMatrix: !!(infoData.leds && infoData.leds.matrix), // Whether it's a 2D matrix setup bootloaderSHA256: infoData.bootloaderSHA256 || '', // Bootloader SHA256 hash brand: infoData.brand, // Device brand (always present) product: infoData.product, // Product name (always present) flashSize: infoData.flash // Flash size (always present) }; // Add optional fields if available if (infoData.psram !== undefined) upgradeData.psramSize = infoData.psram; // Note: partitionSizes not currently available in /json/info endpoint // Make AJAX call to postUpgradeEvent API return fetch('https://usage.wled.me/api/usage/upgrade', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(upgradeData) }); }) .then(res => { if (res.ok) { showToast('Thank you for reporting!'); - updateVersionInfo(info.ver, false); + updateVersionInfo(infoData.ver || '', false); } else { showToast('Report failed. Please try again later.', true); // Do NOT update version info on failure - user will be prompted again } }) .catch(e => { console.log('Failed to report upgrade', e); showToast('Report failed. Please try again later.', true); // Do NOT update version info on error - user will be prompted again }); }And update the call site at Line 3404:
- reportUpgradeEvent(info, oldVersion); + reportUpgradeEvent(oldVersion);Note: You'll need to capture
infoDatain the scope whereupdateVersionInfois called (Line 3461) since it's in a.then()chain. Consider restructuring the promise chain or using an arrow function to maintain the reference.
🧹 Nitpick comments (1)
wled00/data/index.js (1)
3385-3397: Consider DOM APIs to harden against XSS (optional).CodeQL flags the
innerHTMLassignment with template literals as a potential XSS vector. While the interpolated values (title,description,question) are firmware-controlled and not user input, making the exploitability risk very low, using DOM APIs withtextContentfor dynamic parts would eliminate the warning and further harden the code.If you want to address the CodeQL warning, consider constructing the dialog with DOM methods:
// Create dialog structure const heading = d.createElement('h2'); heading.style.cssText = 'margin-top:0;color:var(--c-f);'; heading.textContent = title; // Safe: textContent escapes const descPara = d.createElement('p'); descPara.style.cssText = 'color:var(--c-f);'; descPara.innerHTML = description; // If HTML needed for <strong> tags const questionPara = d.createElement('p'); questionPara.style.cssText = 'color:var(--c-f);'; questionPara.textContent = question; // ... build remaining elements and append to dialogHowever, given the low risk and the fact that version strings are firmware-controlled, this is not critical for the current implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(2 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 (11)
📓 Common learnings
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.
📚 Learning: 2025-11-20T00:04:04.829Z
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.
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-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-08-21T00:00:17.196Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
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-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: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, 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.
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-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/index.js
⏰ 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). (20)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32dev_debug)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- 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)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
wled00/data/index.js (1)
696-697: LGTM! Version check properly integrated.The call to
checkVersionUpgrade(i)is correctly placed afterlastinfo = iinparseInfo(), resolving the previous review concern that the feature was never invoked. The function will now run once per page load when device info is available.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/data/index.js (1)
3309-3472: Version reporting flow and prompt wording match the intended opt‑in, hardware‑only designOnce the path issue above is fixed, the overall feature looks coherent:
- Install vs upgrade is correctly inferred from
version-info.jsonandinfo.ver.- The prompt text clearly frames this as a one‑time hardware report and explicitly excludes personal/behavioral data, which matches the actual payload in
reportUpgradeEvent().- The data sent to
https://usage.wled.me/api/usage/upgradeis limited to device/firmware/hardware fields (deviceId, versions, chip, LED count, matrix flag, bootloader hash, brand/product/flashSize, optional PSRAM), aligning with the non‑behavioral usage‑reporting intent. Based on learnings, this is consistent with the existing deviceId design.Since
index.jswas modified, remember to runnpm run buildbefore producing firmware binaries so the embedded web assets stay in sync.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/data/index.js(2 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 (13)
📓 Common learnings
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: 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: 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/data/** : When modifying web UI files, run `npm run build` to regenerate embedded headers before any firmware build
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/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
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.
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)
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, 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.
📚 Learning: 2025-11-20T00:04:04.829Z
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.
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-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-08-21T00:00:17.196Z
Learnt from: willmmiles
Repo: wled/WLED PR: 4859
File: usermods/udp_name_sync/udp_name_sync.cpp:62-70
Timestamp: 2025-08-21T00:00:17.196Z
Learning: In WLED codebase, UDP packet processing code must never rely on null termination being present on the wire and must always validate packet size first. The codebase already has some vulnerabilities where malformed data can crash the software, so avoiding introduction of new ones is a priority.
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-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: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, 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.
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/**/*.{htm,html,css,js} : Use tabs for indentation in web files (.htm/.html/.css/.js) under wled00/data
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/**/*.cpp : Use 2-space indentation for C++ source files (.cpp)
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/**/!(html_*)*.h : Use 2-space indentation for non-generated C++ header files (.h)
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
⏰ 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). (19)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32c3dev_qio)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: Analyze (c-cpp)
🔇 Additional comments (1)
wled00/data/index.js (1)
696-697: Good place to hook version check into the UI lifecycleCalling
checkVersionUpgrade(i)fromparseInfo()ensures the upgrade/install logic runs once after info load and on subsequent reconnects without extra plumbing, and theversionCheckDoneguard keeps it from re-running on every update. This integration point looks correct.
Both updated. Hopefully good to go? |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| function showVersionUpgradePrompt(info, oldVersion, newVersion) { | ||
| // Determine if this is an install or upgrade |
There was a problem hiding this comment.
Can we test if the update server is accessible here before showing the prompt? It might save a lot of "failed to report" cases if the setup device is also offline.
There was a problem hiding this comment.
Overkill I'd say. It's suppressed already for ap mode, plus to check that connectivity would mean making a connection without consent so definitely not a good idea
There was a problem hiding this comment.
Yeah, it's too bad that we can't directly perform DNS resolution from the browser without connecting to anyone. :( As below I am mostly worried about annoying offline people with repeated prompts.
There was a problem hiding this comment.
You are correct that we could do a hostname lookup only if we are trying to detect internet access rather than connectivity to the usage platform specifically, which would be a GDPR compliant approach.
If folk are always offline, then they can just click never.
Let's see how things go and can always improve based on feedback
There was a problem hiding this comment.
As far as I could find, only Firefox offers a DNS resolution API. Sadly other browsers offer no solution for testing connectivity other than opening a connection somewhere. Maybe someday..
| showToast('Thank you for reporting!'); | ||
| updateVersionInfo(info.ver, false); | ||
| } else { | ||
| showToast('Report failed. Please try again later.', true); |
There was a problem hiding this comment.
Future: we might want to consider saving "user gave permission, but we failed to report" in the version info file. That way we could avoid re-prompting the user with a question they've already answered (and driving more people to click "never again" to just to get it to go away).
There was a problem hiding this comment.
Nice idea, but for the first release let's keep this to a strict simple relationship between consent and action. If it's well received we can consider "lasting consent"
There was a problem hiding this comment.
Fair enough. I'm just concerned that being annoying about reporting will exacerbate any privacy concerns.
New branch to replace the broken co-pilot branch #5098
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.