save global on / off button override in cfg#5591
Conversation
WalkthroughThis PR adds power-button preset configuration to WLED. The feature stores which light preset activates when the button is pressed on versus pressed off, persists the setting in ChangesPower Button Preset Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/data/settings_ui.htm (1)
123-142:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFirst-visit crash:
sett.compdereferencesnullwhenwledUiCfgis absent.When
localStoragehas nowledUiCfgentry,localStorage.getItem('wledUiCfg')returnsnull, andJSON.parse(null)evaluates tonull(no exception is thrown, so thecatchbranch is not entered). The newly addedif (!sett.comp) sett.comp = {};then throwsTypeError: Cannot read properties of null (reading 'comp'), breaking the UI Settings page on a fresh browser/profile. Previously this path was tolerated becauseaddRec(null, …)'sfor…inis a no-op onnull.Normalize
settto an object before touching.comp:🐛 Proposed fix
try { - sett = JSON.parse(sett); + sett = JSON.parse(sett) || {}; } catch (e) { sett = {}; gId('lserr').style.display = "inline"; gId('lserr').innerHTML = "⚠ Settings JSON parsing failed. (" + e + ")"; } if (!sett.comp) sett.comp = {};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/data/settings_ui.htm` around lines 123 - 142, GetLS currently assumes sett is an object after JSON.parse, but JSON.parse(null) returns null and causes a crash when accessing sett.comp; fix by normalizing sett to an object immediately after reading/parsing localStorage (e.g., after sett = JSON.parse(sett) or when sett is falsy) so sett = {} if sett is null/undefined before any access to sett.comp, and keep the existing parse-error UI updates (gId('lserr') usage) intact; update the logic surrounding localStorage.getItem('wledUiCfg'), JSON.parse, and the subsequent if (!sett.comp) sett.comp = {} to ensure sett is always an object in the GetLS function.
🧹 Nitpick comments (1)
wled00/data/index.js (1)
656-662: ⚡ Quick winAvoid persisting
cfgto localStorage on everyparseInfocall.
parseInfo()runs on every WebSocket info update and everyrequestJson()response, so this branch serializes and writes the entirecfgobject on every refresh — even wheni.pon/i.pofdid not change. That is needless I/O on the hot path, and thestorageevent will fire for other open tabs on each write. Gate the write on an actual change:♻️ Proposed fix
- if (typeof i.pon === "number") cfg.comp.on = i.pon; - if (typeof i.pof === "number") cfg.comp.off = i.pof; - try { - localStorage.setItem('wledUiCfg', JSON.stringify(cfg)); - } catch (e) { - // ignore localStorage failures - } + let pwrChanged = false; + if (typeof i.pon === "number" && cfg.comp.on !== i.pon) { cfg.comp.on = i.pon; pwrChanged = true; } + if (typeof i.pof === "number" && cfg.comp.off !== i.pof) { cfg.comp.off = i.pof; pwrChanged = true; } + if (pwrChanged) { + try { localStorage.setItem('wledUiCfg', JSON.stringify(cfg)); } catch (e) { /* ignore */ } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/data/index.js` around lines 656 - 662, The current parseInfo handling unconditionally writes cfg to localStorage after assigning cfg.comp.on/off, causing unnecessary I/O and storage events; change parseInfo so it compares the new values of i.pon and i.pof (or the resulting cfg.comp.on/cfg.comp.off) against their previous values and only call localStorage.setItem('wledUiCfg', JSON.stringify(cfg)) when an actual change occurred (i.e., detect delta on i.pon/i.pof or cfg.comp.on/off before serializing), keeping the try/catch block intact to ignore failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wled00/data/settings_ui.htm`:
- Around line 123-142: GetLS currently assumes sett is an object after
JSON.parse, but JSON.parse(null) returns null and causes a crash when accessing
sett.comp; fix by normalizing sett to an object immediately after
reading/parsing localStorage (e.g., after sett = JSON.parse(sett) or when sett
is falsy) so sett = {} if sett is null/undefined before any access to sett.comp,
and keep the existing parse-error UI updates (gId('lserr') usage) intact; update
the logic surrounding localStorage.getItem('wledUiCfg'), JSON.parse, and the
subsequent if (!sett.comp) sett.comp = {} to ensure sett is always an object in
the GetLS function.
---
Nitpick comments:
In `@wled00/data/index.js`:
- Around line 656-662: The current parseInfo handling unconditionally writes cfg
to localStorage after assigning cfg.comp.on/off, causing unnecessary I/O and
storage events; change parseInfo so it compares the new values of i.pon and
i.pof (or the resulting cfg.comp.on/cfg.comp.off) against their previous values
and only call localStorage.setItem('wledUiCfg', JSON.stringify(cfg)) when an
actual change occurred (i.e., detect delta on i.pon/i.pof or cfg.comp.on/off
before serializing), keeping the try/catch block intact to ignore failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 155ddf11-0226-46c5-8100-a126e3b6ac69
📒 Files selected for processing (6)
wled00/cfg.cppwled00/data/index.jswled00/data/settings_ui.htmwled00/json.cppwled00/wled.hwled00/xml.cpp
@DedeHai yes I see your concern, too. If configuring different on/off presets per UI instance (=different on your Pad, on your MacBook, on your son's PC) is a feature that people use, we would create a regression. I also agree that a "complete solution" - that also clears the browser's local storage on reset-by-button, plus clears all connected browsers - would be an overkill in terms of complexity. Maybe let's ask users (discord) how they use the feature 🤔
@netmindz @lost-hope @willmmiles @Moustachauve what do you think? |
2 participants
as discussed in #5562 this adds saving of the UI setting in config in addition to localstorage.
@softhack007 I am not too sure this is a good idea - at least as is: it prevents users from using different on/off presets on different devices. This could be solved with a checkmark to use "local override only", ignorie the cfg setting and use localstorage only.
the other way to solve #5562 would be to not store them in config but add a mechanism to tell the browser to clear relevant local storage after a factory reset, I am not sure how to do that though as it requires a variable the persists a reboot so it would need to go into the config or use RTC memory. Clearing browser cache for a UI induced factory reset would be possible by adding some code to settings_sec.htm but doing it for a button called factory reset seems a bit more tricky.
Summary by CodeRabbit