-
-
Notifications
You must be signed in to change notification settings - Fork 4k
Add report version feature #5116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
571ab67
b6f3cb6
49a25af
61f5737
17e91a7
579021f
8bc434b
33411f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -693,6 +693,8 @@ function parseInfo(i) { | |
| // gId("filterVol").classList.add("hide"); hideModes(" ♪"); // hide volume reactive effects | ||
| // gId("filterFreq").classList.add("hide"); hideModes(" ♫"); // hide frequency reactive effects | ||
| // } | ||
| // Check for version upgrades on page load | ||
| checkVersionUpgrade(i); | ||
| } | ||
|
|
||
| //https://stackoverflow.com/questions/2592092/executing-script-elements-inserted-with-innerhtml | ||
|
|
@@ -3304,6 +3306,195 @@ function simplifyUI() { | |
| gId("btns").style.display = "none"; | ||
| } | ||
|
|
||
| // Version reporting feature | ||
| var versionCheckDone = false; | ||
|
|
||
| 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); | ||
| } | ||
| }); | ||
| } | ||
netmindz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| function showVersionUpgradePrompt(info, oldVersion, newVersion) { | ||
| // Determine if this is an install or upgrade | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.. |
||
| 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 = '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.' | ||
|
|
||
| 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> | ||
| `; | ||
netmindz marked this conversation as resolved.
Dismissed
Show dismissed
Hide dismissed
|
||
|
|
||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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).
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. I'm just concerned that being annoying about reporting will exacerbate any privacy concerns. |
||
| // 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'); | ||
|
|
||
netmindz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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); | ||
| }); | ||
| } | ||
|
Comment on lines
3312
to
3496
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Version reporting flow is coherent; update prompt wording and consider minor cleanups The end‑to‑end flow — detecting install/upgrade via A few focused points to address or consider:
To simplify the API and avoid dependence on the outer - 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
🧰 Tools🪛 GitHub Check: CodeQL[failure] 3385-3397: Client-side cross-site scripting
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please update your comment to reflect the current code @coderabbitai
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @netmindz, I've reviewed the current code in the diff and my observations remain accurate. Here's an updated assessment: Current State AnalysisThe 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 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 4. Build ProcessRemember to run 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 |
||
|
|
||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| size(); | ||
| _C.style.setProperty('--n', N); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure this wont break anything else in the UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see anything obvious. just wondering why it was commented. Mayb just to save a few bytes.