Upgrade reporting #286
Upgrade reporting #286
Conversation
WalkthroughAdds client-side version-check/reporting with an install/upgrade modal and persistence; injects a build-time WEB_BUILD_TIME macro; introduces ETag-based HTTP caching; adds a streamed OTA subsystem with bootloader SHA‑256 support and OTA validation; corrects PSRAM JSON key and extends ESP32 fingerprinting; exposes AnimArtrix build flags. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Device as WLED_Device
participant Analytics as usage.wled.me
Browser->>Device: GET /json/info (requestJson)
Device-->>Browser: JSON info (currentVersion, hw fields)
Browser->>Browser: checkVersionUpgrade(info) if not versionCheckDone
Browser->>Device: GET /edit?edit=/version-info.json
alt version-info exists
Device-->>Browser: version-info.json (oldVersion, neverAsk)
else 404 / missing
Browser-->>Browser: treat as first-install (oldVersion = "")
end
Browser->>Browser: compare oldVersion vs currentVersion and neverAsk
alt show prompt
Browser->>User: showVersionUpgradePrompt(oldVersion,newVersion)
User-->>Browser: selects action (Yes / Not Now / Never Ask)
alt Yes (report)
Browser->>Device: GET /json/info (fresh)
Device-->>Browser: fresh info
Browser->>Analytics: POST /api/usage/upgrade (UpgradeEventRequest)
alt Analytics 200
Analytics-->>Browser: OK
Browser->>Device: POST /upload (persist version-info.json with newVersion)
else Analytics error
Analytics-->>Browser: error (do not update stored version)
end
else Never Ask
Browser->>Device: POST /upload (persist newVersion + neverAsk=true)
else Not Now
Browser-->>Browser: dismiss
end
else no action
Browser-->>Browser: no prompt
end
Browser->>Browser: set versionCheckDone = true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
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 |
|
still missing:
|
![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 (3)
wled00/data/index.js (3)
3732-3737: Consider notifying user of version check failures.When the version check fails (line 3732), the error is only logged to console. While saving the current version for next time is good, users may benefit from knowing the check failed, especially if it's due to filesystem issues.
Consider adding a toast notification:
.catch(e => { console.log('Failed to load version-info.json', e); + showToast('Unable to check for version updates', false); // On error, save current version for next time if (info && info.ver) { updateVersionInfo(info.ver, false); } });
3744-3775: Improve accessibility and keyboard navigation.The modal dialog lacks accessibility attributes and keyboard navigation support. Users relying on screen readers or keyboard navigation may have difficulty interacting with this prompt.
Add ARIA attributes and ESC key handler:
const overlay = d.createElement('div'); overlay.id = 'versionUpgradeOverlay'; + overlay.setAttribute('role', 'dialog'); + overlay.setAttribute('aria-modal', 'true'); + overlay.setAttribute('aria-labelledby', 'versionUpgradeTitle'); 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);'; + // Handle ESC key + const closeOnEsc = (e) => { + if (e.key === 'Escape' && d.body.contains(overlay)) { + d.body.removeChild(overlay); + d.removeEventListener('keydown', closeOnEsc); + } + }; + d.addEventListener('keydown', closeOnEsc); + // Build contextual message based on install vs upgrade const title = isInstall ? '🎉 Thank you for installing WLED-MM!' : '🎉 WLED-MM Upgrade Detected!'; const description = isInstall ? `You are now running WLED-MM <strong>${newVersion}</strong>.` : `Your WLED-MM has been upgraded from <strong>${oldVersion}</strong> to <strong>${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> + <h2 id="versionUpgradeTitle" 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>Also update event listeners to clean up the ESC handler:
gId('versionReportYes').addEventListener('click', () => { reportUpgradeEvent(oldVersion, newVersion); d.body.removeChild(overlay); + d.removeEventListener('keydown', closeOnEsc); });
3864-3871: Silent failure when updating version info.If the
/uploadrequest fails (lines 3869-3870), the error is only logged to console. Users won't know that their "Never Ask" preference failed to save, and they may be prompted again unexpectedly.Consider showing a toast on failure:
.catch(e => { console.log('Failed to update version-info.json', e); + showToast('Failed to save preferences', true); });
📜 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)
wled00/data/**/*.{htm,html,css,js}: Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
After editing web UI files, run npm run build to regenerate embedded headers
Manually validate web UI changes via local HTTP server and browser
Files:
wled00/data/index.js
wled00/data/**/*.{htm,html,js,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Check the browser console for JavaScript errors after web UI changes
Files:
wled00/data/index.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,js,css} : Check the browser console for JavaScript errors after web UI changes
Applied to files:
wled00/data/index.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : Manually validate web UI changes via local HTTP server and browser
Applied to files:
wled00/data/index.js
🔇 Additional comments (5)
wled00/data/index.js (5)
2232-2232: LGTM - Good integration point.The placement of
checkVersionUpgrade(i)afterparseInfo(i)ensures device information is available for version checking and reporting.
3806-3822: Remove premature approval stamps — backend field validation required before approval.The review comment contains
andtags before verification is actually complete. The requested verification of whetherproduct,flash, andtpramfields exist in the/json/infoendpoint cannot be performed due to repository access constraints.Blockers preventing verification:
- Backend
/json/infoendpoint implementation cannot be accessed to confirm field availabilityinfoData.product,infoData.flash, andinfoData.tpramexistence in the response must be validated in C++ code before this JavaScript implementation can be approved- Coding guideline checks (tab indentation, npm build, browser console errors) cannot be verified
Before approval, verify:
- Backend: Confirm
/json/inforesponse includesproduct,flash, andtpramfields- Frontend: Ensure
wled00/data/index.jsuses tabs (not spaces) for indentation- Build: Run
npm run buildand confirm no browser console JavaScript errors- Remove approval stamps until verification is complete
3699-3711: Based on my web search results, I can now evaluate the review comment:The
/edit?edit=version-info.jsonendpoint is WLED's documented standard pattern for file accessThe web search confirms that:
/editis WLED's file editor/manager endpoint for accessing device files (version-info.json, presets.json, cfg.json, etc.)- The query parameter
?edit=filenameis the standard WLED way to specify which file to retrieve- This pattern is documented in WLED's mapping documentation and API reference
However, the search also reveals that WLED provides dedicated JSON API endpoints (
/json/info,/json) that are arguably more appropriate for version checking, but the current approach using/edit?edit=version-info.jsonis still valid and documented.
The endpoint pattern is standard and correctly documented, not non-standard as stated in the review comment.
The
/edit?edit=version-info.jsonendpoint is the documented WLED pattern for file access. The review comment's primary concern—that this is a non-standard endpoint—is incorrect. While WLED also offers dedicated JSON API endpoints like/json/infofor version information, using/editfor this purpose is valid and follows WLED's documented file access patterns.
3856-3863: The review comment is incorrect. The FormData field name 'data' is correct for the WLED /upload endpoint.Web search confirms that the WLED /upload endpoint expects the multipart form field to be named "data", not "upload". Official WLED documentation and community sources (WLED Discourse, Reddit) show the correct usage as:
curl -F "data=@file" http://WLED-IP/uploadThe code at lines 3856-3863 correctly uses
formData.append('data', blob, 'version-info.json'). The suggested change to'upload'would break the upload functionality.Likely an incorrect or invalid review comment.
3806-3822: Unable to confirm API requirement claim; code structure suggests field is optional.Repository access failed and web searches found no published API schema specifying
bootloaderSHA256as a required field. However, the code at line 3814 uses an empty string fallback (infoData.bootloaderSHA256 || ''), which is the standard pattern for optional fields in JavaScript, not required ones. If the backend API strictly required this field, sending an empty string would fail validation—the fact that the code is written this way suggests the field is either:
- Optional in the backend API, or
- The backend hasn't yet enforced validation for it
The comment accurately states the field "is not yet availeable in WLEDMM," but the review's assertion that this "will cause API validation errors" cannot be verified without access to the actual API schema or server implementation.
| return fetch('https://usage.wled.me/api/usage/upgrade', { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| }, | ||
| body: JSON.stringify(upgradeData) | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout and improve error handling for external API call.
The fetch request to https://usage.wled.me/api/usage/upgrade has no timeout and could hang indefinitely. Additionally, the hardcoded external URL may face CORS restrictions depending on the server configuration.
Add a timeout and better error context:
// Make AJAX call to postUpgradeEvent API
- return fetch('https://usage.wled.me/api/usage/upgrade', {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 10000); // 10 second timeout
+
+ return fetch('https://usage.wled.me/api/usage/upgrade', {
method: 'POST',
headers: {
'Content-Type': 'application/json'
},
- body: JSON.stringify(upgradeData)
+ body: JSON.stringify(upgradeData),
+ signal: controller.signal
+ }).finally(() => {
+ clearTimeout(timeoutId);
});
})
.then(res => {
if (res.ok) {
showToast('Thank you for reporting!');
updateVersionInfo(newVersion, false);
} else {
- showToast('Report failed. Please try again later.', true);
+ showToast(`Report failed (${res.status}). 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);
+ const isTimeout = e.name === 'AbortError';
+ console.log('Failed to report upgrade', e);
+ showToast(isTimeout ? 'Report timed out. Please try again later.' : 'Report failed. Please try again later.', true);
// Do NOT update version info on error - user will be prompted again
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wled00/data/index.js around lines 3825-3831, the fetch to
https://usage.wled.me/api/usage/upgrade has no timeout and lacks robust error
handling and CORS consideration; wrap the call in an async try/catch, use an
AbortController with a configurable timeout (clear the timeout on completion),
check response.ok and parse/attach response body or status to any thrown error,
and surface/log the enriched error context; additionally make the URL
configurable (env/config) or move the request to a server-side endpoint to avoid
client-side CORS issues.
If you want to capture more, please open an issue against the wled_usage repo. Am I not capturing the flash size already? |
Add WEB_BUILD_TIME to html_ui.h and use it for ETag generation Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: DedeHai <[email protected]> Co-authored-by: Aircoookie <[email protected]>
OK
Yes you're taking the flash size, but not the filesystem size. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tools/cdata.js (1)
116-155: Build-time WEB_BUILD_TIME macro injection looks good; optional: compute timestamp once per run
generateBuildTime()and the injectedWEB_BUILD_TIMEblock correctly give the C++ side a stable UNIX timestamp for cache-busting and ETag generation, and the#ifdef/#undefguard prevents redefinition issues when multiple html headers are included. To guarantee all generated headers share an identical timestamp even if the script runs longer than a second, you could compute it once and reuse it:-// Generate build timestamp as UNIX timestamp (seconds since epoch) -function generateBuildTime() { - return Math.floor(Date.now() / 1000); -} +// Generate build timestamp as UNIX timestamp (seconds since epoch) +const BUILD_TIME = Math.floor(Date.now() / 1000); +function generateBuildTime() { + return BUILD_TIME; +}wled00/wled_server.cpp (2)
491-493: Call to setStaticContentCacheHeaders() on 404 is currently a no-opBecause
setStaticContentCacheHeaders()immediately returns whencode != 200, the call with404here has no effect and can be removed for clarity unless you plan to extend the helper to also handle non-200 responses:response->addHeader(FPSTR(s_content_enc),"gzip"); - setStaticContentCacheHeaders(response, 404); request->send(response);
727-728: Settings pages now emit ETag; consider optional 304 handling for bandwidth savingsSettings HTML now gets
Cache-ControlandETagviasetStaticContentCacheHeaders(), butserveSettings()never callshandleIfNoneMatchCacheHeader(), so these responses will always be 200 with a full body even when the client sends a matchingIf-None-Match. If you ever want conditional GETs for these pages too, you could add an early:if (handleIfNoneMatchCacheHeader(request)) return;before constructing the
AsyncWebServerResponse*, at least for the fully static subpages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tools/cdata.js(2 hunks)wled00/wled_server.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/wled_server.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
tools/cdata.jswled00/wled_server.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,css,js} : After editing web UI files, run npm run build to regenerate embedded headers
Applied to files:
tools/cdata.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: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32dev_compat)
🔇 Additional comments (2)
wled00/wled_server.cpp (2)
19-62: ETag generation and cache header helpers are coherent and safeUsing
WEB_BUILD_TIME,cacheInvalidate, and an optionaleTagSuffixto build the ETag string, and centralizing cache headers insetStaticContentCacheHeaders(), gives you a clear, predictable versioning story. The 200-only guard for ETag emission and the 304 handling inhandleIfNoneMatchCacheHeader()are consistent and don’t introduce correctness issues.
512-524: Index route’s conditional GET handling with ETag looks correctThe
//index path now cleanly short‑circuits onIf-None-MatchviahandleIfNoneMatchCacheHeader(), and otherwise serves the gzipped PROGMEM page withCache-ControlandETagset bysetStaticContentCacheHeaders(). This matches the new caching model without altering existing control flow.
* reduce memory footprint by removing all unneeded functions in ota_update.cpp * don't compile ota_update.cpp when WLED_DISABLE_OTA is defined
small update for code readability
flash size limits exceeded
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
wled00/data/index.js (1)
3795-3847: External upgrade-report POST still lacks timeout/error enrichmentThe
reportUpgradeEventPOST tohttps://usage.wled.me/api/usage/upgradeis still a barefetchwithout timeout or richer error context:return fetch('https://usage.wled.me/api/usage/upgrade', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(upgradeData) }); ... .catch(e => { console.log('Failed to report upgrade', e); showToast('Report failed. Please try again later.', true); });As noted in a previous review, this can hang indefinitely on bad networks and gives users very little insight into what went wrong. A small wrapper with
AbortControllerand differentiated messaging (timeout vs other errors, include status code on non‑OK) would make this much more robust, especially since it targets an external origin.Given you’ve already seen this feedback and may have consciously deferred it, consider it a “when convenient” hardening task rather than a blocker.
What is the recommended way to implement a timeout around the Fetch API (e.g., using AbortController) in modern browsers?
🧹 Nitpick comments (5)
wled00/data/simple.js (1)
520-536: Remember to rebuild embedded assets and check browser consoleSince this file lives under
wled00/data/*.js, per project guidelines you should:
- Run the web UI build (
npm run build) so the embedded HTML/JS headers stay in sync.- Manually open the simple UI in a browser and confirm there are no new JS errors or layout issues in the console, especially around the updated PSRAM rows.
wled00/ota_update.h (1)
3-8: OTA header surface is fine; static helper declarations are internal-onlyThe header cleanly:
- Avoids exposing the legacy Async OTA API by wrapping it in
#if 0.- Limits the bootloader SHA helpers to
#if defined(ARDUINO_ARCH_ESP32) && !defined(WLED_DISABLE_OTA)so non-ESP32 builds aren’t affected.- Declares
calculateBootloaderSHA256andinvalidateBootloaderSHA256Cacheasstatic, which is appropriate if their definitions inota_update.cppalso usestatic(internal linkage).As long as the definitions in
ota_update.cppare alsostatic, this won’t violate ODR and keeps the internal helpers out of the global symbol table. If they are currently non-static there, please make themstaticto match these declarations. The Clang “Arduino.h not found” error is a tooling configuration issue, not a code bug.Also applies to: 58-76
wled00/data/index.js (1)
516-555: Apply UI build pipeline and console checks for the new version-reporting flowBecause
index.jsis part of the web UI bundle:
- Re-run the UI build (
npm run build) so any generatedhtml_*.hassets pick up the new version-reporting logic.- Test the main UI in a browser against an ESP32 target with PSRAM and OTA enabled:
- confirm
/json/infocontainsbootloaderSHA256andtpsramas expected,- exercise the “Yes / Not Now / Never Ask” paths,
- and watch the browser console for CORS or JavaScript errors from
checkVersionUpgrade,reportUpgradeEvent, andupdateVersionInfo.This will catch any subtle integration issues that static review can’t see.
Also applies to: 3690-3871
wled00/ota_update.cpp (2)
38-83: Note: Syntax error in disabled code block.If this code is ever re-enabled, line 83 is missing a semicolon after the struct definition:
-} +};Not urgent since the block is currently disabled via
#if 0.
326-329: Unused static function cannot be called externally.
invalidateBootloaderSHA256Cache()is declaredstatic, preventing calls from other translation units. The comment suggests it should be called "after bootloader update," which would likely occur in other files.Either:
- Remove
staticand declare the function inota_update.hif external use is intended, or- Remove the function if it's truly unnecessary (bootloader updates are rare and typically require a reboot anyway).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)platformio.ini(1 hunks)wled00/data/index.js(3 hunks)wled00/data/simple.js(1 hunks)wled00/json.cpp(5 hunks)wled00/ota_update.cpp(1 hunks)wled00/ota_update.h(1 hunks)wled00/util.cpp(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (3)
wled00/data/**/*.{htm,html,css,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
wled00/data/**/*.{htm,html,css,js}: Use tabs for indentation in web UI files (.htm/.html/.css/.js) under wled00/data
After editing web UI files, run npm run build to regenerate embedded headers
Manually validate web UI changes via local HTTP server and browser
Files:
wled00/data/simple.jswled00/data/index.js
wled00/data/**/*.{htm,html,js,css}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Check the browser console for JavaScript errors after web UI changes
Files:
wled00/data/simple.jswled00/data/index.js
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/json.cppwled00/util.cppwled00/ota_update.cppwled00/ota_update.h
🧠 Learnings (10)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-10-22T21:50:25.839Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: usermods/usermod_v2_rotary_encoder_ui_ALT/usermod_v2_rotary_encoder_ui_ALT.h:155-157
Timestamp: 2025-10-22T21:50:25.839Z
Learning: In WLED-MM PR #270, only Effect (Mode) IDs are migrated to 16-bit; Palette IDs and counts remain 8-bit. Usermod code should widen mode-related indices/loops to uint16_t while keeping palette-related indices/loops as uint8_t.
Applied to files:
wled00/data/simple.jswled00/json.cpp
📚 Learning: 2025-11-02T22:07:15.689Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-02T22:07:15.689Z
Learning: In MoonModules/WLED-MM, WLEDMM_FASTPATH will become the default code path. Code inside "#ifndef WLEDMM_FASTPATH" blocks represents deprecated paths that are being phased out and should be ignored during reviews.
Applied to files:
wled00/data/simple.js
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/data/**/*.{htm,html,js,css} : Check the browser console for JavaScript errors after web UI changes
Applied to files:
wled00/data/index.js
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
platformio.iniwled00/json.cpp
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/json.cpp
📚 Learning: 2025-11-24T00:15:38.069Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-24T00:15:38.069Z
Learning: In MoonModules/WLED-MM, when reading ESP32/ESP32-S3 eFuse blocks using esp_efuse_read_field_blob(): EFUSE_BLK0 has only 7 × 32-bit registers (224 bits total), not 8 registers (256 bits). Reading more than 7 words from EFUSE_BLK0 triggers assertion failure in esp_efuse_utility_read_reg(). Use maximum 7 words when reading EFUSE_BLK0 to avoid crashes. This applies to all ESP32 variants (ESP32, ESP32-S2, ESP32-S3, ESP32-C3).
Applied to files:
wled00/json.cpp
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
wled00/json.cpp
📚 Learning: 2025-07-12T04:20:14.546Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 253
File: usermods/audioreactive/audio_source.h:1256-1261
Timestamp: 2025-07-12T04:20:14.546Z
Learning: In WLED AudioReactive usermod, DC offset removal optimization in DMAadcSource::getSamples() should be implemented after PR #248 merges, which will change the sample buffer from float to int16. This eliminates the need for temporary integer buffers and allows direct integer arithmetic on the native buffer type for better memory efficiency on resource-constrained MCUs like C3 and S2.
Applied to files:
wled00/util.cpp
🧬 Code graph analysis (2)
wled00/json.cpp (1)
wled00/ota_update.cpp (2)
getBootloaderSHA256Hex(321-324)getBootloaderSHA256Hex(321-321)
wled00/ota_update.cpp (1)
wled00/wled.h (1)
instance(930-934)
🪛 Clang (14.0.6)
wled00/ota_update.h
[error] 3-3: 'Arduino.h' file not found
(clang-diagnostic-error)
⏰ 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: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_S)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
🔇 Additional comments (6)
wled00/data/simple.js (1)
525-533: PSRAM rows look consistent with newtpsramfieldThe changed PSRAM info rows correctly align with the new JSON field:
i.tpsramis now the total PSRAM in bytes; dividing by 1024 and showing kB is consistent with thejson.cppproducer.- “Max Used PSRAM” uses
i.tpsram - i.psusedramand switches to kB when above 16 kB, which matches the previous behavior while fixing the key name.Given
json.cpponly setspsram,psusedram, andtpsramtogether underBOARD_HAS_PSRAM, the guard oni.psusedramis sufficient.wled00/json.cpp (2)
1-3: Bootloader SHA256 reporting is wired correctly and gated by OTA/ESP32Including
ota_update.hat the top and exposingbootloaderSHA256only under:#if defined(ARDUINO_ARCH_ESP32) ... #ifndef WLED_DISABLE_OTA root[F("bootloaderSHA256")] = getBootloaderSHA256Hex(); #endif #else // ESP8266... #endifis a clean way to surface the digest to
/json/infowithout impacting ESP8266 or OTA-disabled builds. The header itself also gates the declaration ofgetBootloaderSHA256Hex()onARDUINO_ARCH_ESP32 && !WLED_DISABLE_OTA, so the symbol is only required where it exists.No issues here from a correctness or linkage perspective.
Also applies to: 1085-1090
1109-1127:tpsramJSON key matches the UI expectationsThe PSRAM reporting block:
#if defined(ARDUINO_ARCH_ESP32) && defined(BOARD_HAS_PSRAM) if (psramFound()) { root[F("tpsram")] = ESP.getPsramSize(); root[F("psram")] = ESP.getFreePsram(); root[F("psusedram")] = ESP.getMinFreePsram(); ... } #else // commented-out test values #endifuses
tpsramas “total PSRAM in bytes”, withpsramandpsusedramas current and min free. That lines up with the consumers in bothsimple.jsandindex.jswhich expecti.tpsramto be the total and derive used values from it.This looks consistent and backward compatible (all three keys remain under the same
psramFound()guard).wled00/util.cpp (1)
10-10: Based on my verification through documentation searches, I've found critical information about the ESP-IDF ADC calibration API. Let me provide the rewritten review comment:
Array bounds concern is incorrect; return value handling remains a valid consideration
The ADC calibration data concern needs correction:
The
low_curveandhigh_curvepointers are documented 20-element lookup tables (LUT_POINTS = 20), not unspecified length. Reading 8 entries unconditionally is safe and poses no out-of-bounds risk. This information is available inesp_adc_cal_legacy.cand the ESP-IDF documentation.The second concern about unchecked
esp_adc_cal_characterize()return value remains valid from an API robustness perspective. The function can returnESP_ADC_CAL_VAL_NOT_SUPPORTED,ESP_ADC_CAL_VAL_DEFAULT_VREF,ESP_ADC_CAL_VAL_EFUSE_VREF, orESP_ADC_CAL_VAL_EFUSE_TP. Per ESP-IDF guidance, the return value should be checked to determine whether the calibration succeeded and which data (coeff_a/b vs. curves) is valid. Ignoring it and unconditionally trustinglow_curve/high_curvewhenNOT_SUPPORTEDorDEFAULT_VREFis returned could use unpopulated or invalid data.Suggested action:
Check the return value ofesp_adc_cal_characterize()and only uselow_curve/high_curvewhen the calibration mode indicates they are available (e.g.,ESP_ADC_CAL_VAL_EFUSE_VREForESP_ADC_CAL_VAL_EFUSE_TP). For unsupported or default-vref cases, skip the curve mixing or fall back to coeff-only mixing.Likely an incorrect or invalid review comment.
wled00/ota_update.cpp (2)
1-36: Well-structured platform configuration.The bootloader offset constants are appropriately documented with the upstream reference, and the multi-platform support covers the relevant ESP32 variants thoroughly.
308-318: LGTM!SHA256 finalization and hex conversion are implemented correctly. The explicit null termination on line 316 is redundant but harmless.
| if (json.info) { | ||
| let i = json.info; | ||
| parseInfo(i); | ||
| checkVersionUpgrade(i); // Check for version upgrade | ||
| populatePalettes(i); | ||
| if (isInfo) populateInfo(i); | ||
| } |
There was a problem hiding this comment.
version-info.json is likely never saved (/upload field name mismatch, no loc support)
The version-reporting flow is well thought out overall, but there are two concrete issues that will likely break persistence and file-mode behavior:
- Wrong form field name for
/upload
Existing code uses uploadFileWithText() to POST files to /upload:
function uploadFileWithText(name, text) {
...
var formData = new FormData();
var blob = new Blob([text], {type : 'application/text'});
var fileOfBlob = new File([blob], name);
formData.append("upload", fileOfBlob);
req.send(formData);
}The new updateVersionInfo() instead does:
const blob = new Blob([JSON.stringify(versionInfo)], { type: 'application/json' });
const formData = new FormData();
formData.append('data', blob, 'version-info.json');
fetch('/upload', {
method: 'POST',
body: formData
})The server-side upload handler in WLED expects the field name "upload" (as evidenced by uploadFileWithText); sending "data" means the firmware will not recognize or store the file. As a result:
version-info.jsonwill never be created/updated.checkVersionUpgrade()will keep treating this as a first install or as a version mismatch, so users will be re-prompted indefinitely.
You should either:
- Reuse the existing helper:
-function updateVersionInfo(version, neverAsk) {
- const versionInfo = { version, 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('/upload', { method: 'POST', body: formData });
-}
+function updateVersionInfo(version, neverAsk) {
+ const versionInfo = { version, neverAsk };
+ uploadFileWithText('/version-info.json', JSON.stringify(versionInfo));
+}- Or at least change the field name to
"upload"and ensure the filename path matches what/edit?edit=version-info.jsonexpects.
- Hard‑coded device-local URLs ignore
loc/locip
Other fetches in this file consistently use (loc ? \http://${locip}` : '')as a prefix so the UI works both when served by the device and when opened fromfile:`. The new logic uses bare paths:
fetch('/edit?edit=version-info.json', ...)fetch('/json/info', ...)fetch('/upload', ...)
In file‑mode these will target the local filesystem origin instead of the WLED device, so:
checkVersionUpgrade()will fail its fetch, fall into thecatch, and attempt to POST/uploadto the wrong origin.reportUpgradeEvent()will fail to fetch/json/infoat all.
If you care about file‑mode parity, you should follow the same pattern used elsewhere:
-function checkVersionUpgrade(info) {
+function checkVersionUpgrade(info) {
+ const base = loc ? `http://${locip}` : '';
...
- fetch('/edit?edit=version-info.json', { method: 'get' })
+ fetch(base + '/edit?edit=version-info.json', { method: 'get' })
...and similarly for /json/info and /upload.
Once those two points are fixed, the flow (single per-load check, honor neverAsk, prompt on real version changes, and build an upgradeData payload from /json/info) looks structurally sound.
Also applies to: 3690-3871
There was a problem hiding this comment.
@coderabbitai i have tested the code (latest commit of this PR) on several boards, and version-info.json is properly created on the device, and it is also loaded and recognized by the JS function.
please re-check your analysis, I think you made some wrong assumptions.
There was a problem hiding this comment.
@softhack007, thank you for testing and providing that feedback! I apologize for the incorrect analysis. If version-info.json is being properly created and recognized on the device, then my assumptions about the /upload endpoint were clearly wrong.
I'm curious about one thing for my own learning: I noticed that the new updateVersionInfo() function uses 'data' as the FormData field name:
formData.append('data', blob, 'version-info.json');While the existing uploadFileWithText() function uses 'upload':
formData.append("upload", fileOfBlob);Does the WLED firmware /upload endpoint accept both field names, or does it perhaps just look at the filename regardless of the field name? Understanding this would help me avoid similar misassumptions in the future.
In any case, I appreciate you taking the time to test this thoroughly—empirical evidence always beats theoretical analysis!
🧠 Learnings used
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
|
@netmindz I have tested the upgrade reporting procedure in several boards, it I think its working now. I don't know much about JS - maybe you want to check the recommendations made by the bunny? Just give me your OK and i'll merge the PR to mdev. |
* ``-DCONFIG_LITTLEFS_FOR_IDF_3_2 -DLFS_THREADSAFE`` caused a crash when mounting the filesystem * added "extends = " to MM legacy buildenvs * make sure that flash_mode is always well-defined
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wled00/wled_server.cpp (1)
492-492: No-op call tosetStaticContentCacheHeaderswith 404.This call passes
404as the status code, butsetStaticContentCacheHeadersreturns early for any non-200 status (line 33). This makes the call effectively a no-op.This is addressed by the earlier suggestion to revise the
setStaticContentCacheHeadersAPI design.
🧹 Nitpick comments (2)
wled00/wled_server.cpp (2)
19-21: Unused constants_expires.The
s_expiresPROGMEM string is defined but never referenced in this file. Line 623 usesF("Expires")directly instead ofFPSTR(s_expires).Apply this diff to use the PROGMEM constant or remove it:
response->addHeader(F("Cache-Control"),"no-store"); -response->addHeader(F("Expires"),"0"); +response->addHeader(FPSTR(s_expires),"0");Or remove the unused constant if it's not needed elsewhere.
31-46: Consider simplifying the API or handling non-200 codes.The
setStaticContentCacheHeadersfunction accepts acodeparameter but only processescode == 200(line 33). The call at line 492 passes404, which results in an early return and no headers being set.Consider either:
- Removing the
codeparameter and only calling this function for 200 responses.- Setting appropriate cache headers for other status codes (e.g., 404 responses could be cached).
The current design is functional but may confuse future maintainers who expect the function to handle multiple status codes based on its signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platformio.ini(2 hunks)wled00/json.cpp(5 hunks)wled00/wled_server.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/json.cpp
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/wled_server.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/wled_server.cpp
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
Repo: MoonModules/WLED-MM PR: 248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
platformio.ini
📚 Learning: 2025-11-14T13:55:44.442Z
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 0
File: :0-0
Timestamp: 2025-11-14T13:55:44.442Z
Learning: In MoonModules/WLED-MM, always flag Variable Length Arrays (VLA) during code reviews, especially when they could overflow available stack space (typically 2K-4K on ESP32/ESP8266). VLAs based on dynamic parameters like segment dimensions, particle counts, or pixel buffers should be highlighted and require justification from the author. Prefer fixed-size arrays, heap allocation, or bounded VLAs with explicit maximum size constraints.
Applied to files:
platformio.ini
📚 Learning: 2025-06-30T17:53:37.232Z
Learnt from: DedeHai
Repo: MoonModules/WLED-MM PR: 248
File: usermods/audioreactive/audio_reactive.h:199-200
Timestamp: 2025-06-30T17:53:37.232Z
Learning: In WLED AudioReactive usermod, when using ArduinoFFT (enabled via UM_AUDIOREACTIVE_USE_ARDUINO_FFT), it's acceptable and necessary to redefine the global sqrt macro to sqrtf within the conditional compilation block for performance optimization. This is a specific requirement for ArduinoFFT optimization that can provide 10-50% performance improvement on ESP32.
Applied to files:
platformio.ini
⏰ 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: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (esp8266_4MB_S)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_V4_S)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32_4MB_M_eth)
- GitHub Check: Builds (esp32_4MB_V4_HUB75_forum)
- GitHub Check: Builds (esp32_16MB_V4_S)
- GitHub Check: Builds (esp32_pico_4MB_V4_S)
- GitHub Check: Builds (esp32dev_compat)
- GitHub Check: Builds (esp32_16MB_V4_M_debug)
- GitHub Check: Builds (esp32_4MB_PSRAM_S)
🔇 Additional comments (4)
platformio.ini (2)
1267-1271: Variable substitution syntax is now correct.The previous syntax error using braces
{}instead of${}has been successfully remediated. AnimArtrix build flags and lib_ignore entries are now properly expanded via PlatformIO variable substitution. The logic of disabling AnimArtrix in 4MB environments due to flash constraints is sound.
2885-2886: Consistent AnimArtrix flag removal for memory-constrained environments.The athom_music environment correctly removes AnimArtrix build flags using the same proper
${}substitution syntax as the 4MB base environments. Comment clarity indicates deliberate flash-saving optimization.wled00/wled_server.cpp (2)
48-62: LGTM! Correct HTTP caching implementation.The conditional request handling correctly implements RFC 7232:
- Compares
If-None-Matchheader with the generated ETag- Returns
304 Not Modifiedwith appropriate headers when the ETag matches- Returns
falseto allow normal response flow when there's no match
27-29: TheWEB_BUILD_TIMEmacro does not exist in the WLED or WLED-MM repositories.After comprehensive searching:
- GitHub search of MoonModules/WLED-MM: no matches for
WEB_BUILD_TIME- GitHub search of upstream wled/WLED: no matches for
WEB_BUILD_TIME- Web search of WLED documentation and codebase: no references found
The review comment requests verification of a macro that does not appear to be defined anywhere in the project. If the code shown actually uses
WEB_BUILD_TIMEwithout it being defined, compilation would fail—but since this macro is not found in either repository, the review comment's premise appears to be incorrect.Likely an incorrect or invalid review comment.
small flash optimization by re-using string constants (string merging)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wled00/wled_server.cpp (3)
27-46: ETag generation and conditional response helpers look correct
generateEtag()’s format ("%u-%02x-%04x") and a 32‑byte buffer are comfortably sized forWEB_BUILD_TIME,cacheInvalidate, and a 16‑bit suffix, so no overflow risk with the current format.handleIfNoneMatchCacheHeader()correctly:
- only short-circuits for “would‑be 200” responses,
- compares against the same ETag string you later emit in
setStaticContentCacheHeaders(), and- returns a 304 with matching cache headers when appropriate.
- Passing
code=200intosetStaticContentCacheHeaders()even when creating a 304 response is a reasonable way to express “ETag and cache policy for a resource whose normal status is 200”.If you ever extend the ETag format, consider
snprintf_Pinstead ofsprintf_Pfor extra future‑proofing, but with the current format this is not a blocker.Also applies to: 48-62
485-493:setStaticContentCacheHeaders(response, 404)currently no-ops
setStaticContentCacheHeaders()early-returns for anycode != 200, so the new call with404on the 404 page does not add eitherCache-ControlorETagand is effectively a no-op.If the intent is:
- no ETag but still cache policy for 404 pages, consider either:
- calling
setStaticContentCacheHeaders(response)with the defaultcode=200, or- adding a dedicated helper for non‑200 error pages that only sets the desired
Cache-Control.
If you deliberately want default caching semantics for 404, the helper call can simply be removed for clarity.
697-729: Reusing static cache headers for settings pages—consider stricter policy for sensitive subpagesWith
setStaticContentCacheHeaders(response)now applied to all settings HTML/CSS (including security-related subpages), those pages are served withCache-Control: no-cache(in non‑debug builds). That allows browsers to store them on disk but forces revalidation.For “normal” settings/welcome pages this is fine, but for pages that surface secrets (WiFi credentials, security PIN, etc.) you might prefer they are not stored at all. Two possible tweaks:
- Skip
setStaticContentCacheHeaders()for security‑sensitive subpages and addCache-Control: no-storeexplicitly, or- Extend the helper (e.g., an extra flag or different
code/mode) so those pages can opt intono-storewhile the rest stay onno-cache.Not a correctness issue, but worth considering from a privacy standpoint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/wled_server.cpp(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.{cpp,h}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use spaces (2 per level) for C++ firmware source and headers in wled00
Files:
wled00/wled_server.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: softhack007
Repo: MoonModules/WLED-MM PR: 270
File: wled00/fcn_declare.h:403-406
Timestamp: 2025-10-22T21:59:24.331Z
Learning: In MoonModules/WLED-MM PR #270, the out-of-bounds null-terminator write in wled00/util.cpp (extractModeSlider) is deferred and tracked in Issue #272; do not address it within PR #270.
📚 Learning: 2025-10-12T21:55:59.766Z
Learnt from: CR
Repo: MoonModules/WLED-MM PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-12T21:55:59.766Z
Learning: Applies to wled00/html_*.h : Do not edit generated embedded web header files: wled00/html_*.h
Applied to files:
wled00/wled_server.cpp
⏰ 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: Builds (esp32c3dev_4MB_M)
- GitHub Check: Builds (adafruit_matrixportal_esp32s3_tinyUF2)
- GitHub Check: Builds (athom_music_esp32_4MB_M)
- GitHub Check: Builds (esp01_1MB_S)
- GitHub Check: Builds (esp8266_4MB_M)
- GitHub Check: Builds (esp8266pro_16MB_M)
- GitHub Check: Builds (esp8266_2MB_S)
- GitHub Check: Builds (esp32c3dev_2MB_M)
- GitHub Check: Builds (esp32S3_WROOM-2_M)
- GitHub Check: Builds (seeed_esp32c3_4MB_S)
- GitHub Check: Builds (esp32c3mini_dio_4MB_M)
- GitHub Check: Builds (esp32s2_PSRAM_S)
- GitHub Check: Builds (esp32s2_PSRAM_M)
- GitHub Check: Builds (esp32S3_8MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_M)
- GitHub Check: Builds (esp32S3_16MB_PSRAM_M_HUB75)
- GitHub Check: Builds (esp32_16MB_V4_M)
- GitHub Check: Builds (esp32S3_4MB_PSRAM_S)
- GitHub Check: Builds (esp32S3_4MB_S)
- GitHub Check: Builds (esp32S3_8MB_S)
🔇 Additional comments (2)
wled00/wled_server.cpp (2)
19-22: Flash string forCache-Controlheader is fineDefining
s_cache_controlin PROGMEM matches the pattern of other header-name constants and is a small but good flash-usage optimisation. The commented-outs_no_store/s_expirescan remain as-is or be dropped later; no functional impact either way.
620-623: Explicitno-storeforserveSettingsJSis appropriateUsing:
response->addHeader(FPSTR(s_cache_control), F("no-store")); response->addHeader(F("Expires"), "0");on the generated settings JS is a good choice: it avoids the browser caching user/device-specific configuration script while still keeping the logic simple.
2 participants
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.