Deliver user preferences in the subscribe_events initial_state snapshot#1479
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1479 +/- ##
=======================================
Coverage 99.50% 99.50%
=======================================
Files 221 222 +1
Lines 17166 17253 +87
=======================================
+ Hits 17081 17168 +87
Misses 85 85
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR moves user preferences delivery to the subscribe_events initial_state snapshot, backed by a new RAM-canonical PreferencesStore that loads once at startup, debounces writes to a dedicated preferences file, and migrates legacy preferences out of the shared .device-builder.json sidecar. This aligns preferences with the repo’s snapshot-plus-events pattern and reduces first-paint round trips.
Changes:
- Add
PreferencesStore(RAM-canonical, debounced persistence) with migration from legacy_preferencessidecar key and corrupt-file preservation behavior. - Include preferences in
subscribe_eventsinitial_state, and wire onboarding/config controllers to read/write through the store. - Update/extend tests and API docs to reflect the new snapshot + persistence behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
esphome_device_builder/device_builder.py |
Loads prefs at startup and adds preferences to the subscribe_events initial snapshot. |
esphome_device_builder/controllers/config/controller.py |
Instantiates and lifecycle-manages the preferences store; serves get/set preferences via RAM snapshot/merge. |
esphome_device_builder/controllers/config/_preferences_store.py |
New RAM-canonical preferences store with debounced writes and legacy migration logic. |
esphome_device_builder/controllers/config/preferences.py |
Reduces legacy preferences module to the sidecar key constant used by migration. |
esphome_device_builder/controllers/config/__init__.py |
Removes legacy preference helpers from exports. |
esphome_device_builder/controllers/onboarding.py |
Switches onboarding state/ack to read/mutate onboarding completion version via the store. |
docs/API.md |
Documents initial_state.preferences and the new persistence/migration semantics. |
tests/test_subscribe_events_cleanup.py |
Stubs config prefs and asserts preferences are present in the initial_state snapshot. |
tests/test_onboarding_controller.py |
Updates onboarding tests to use the preferences store snapshot. |
tests/test_config_controller.py |
Updates config-controller tests around prefs behavior; adds a load/flush smoke test. |
tests/controllers/config/test_preferences_store.py |
New dedicated test suite for store load/migration/corruption behaviors. |
tests/controllers/config/__init__.py |
Establishes a test package for config-controller tests. |
|
|
esphbot
left a comment
There was a problem hiding this comment.
No blocking issues found.
|
Addressed the strip-abort suggestion in 84ae3bb: The two notes are intentional for this PR: the stale-key-never-re-stripped case is the same benign situation (dedicated file wins), and the per-mutation bus event for cross-tab propagation belongs to the experience-levels follow-up, not this plumbing extraction. Also handled the Copilot inline notes (assert to raise for the config narrowing, single-line test docstring). Left the debounce-write and |
|
Following up on the Silent Failure Analysis (missed it in my last reply):
|
PR Review — Deliver user preferences in the subscribe_events initial_state snapshotClean snapshot-plus-events extraction with genuinely crash-safe migration. Merge-ready. What's solid:
Nothing needs attention to merge. The remaining accepted tradeoffs (silent success when persistence is disabled, fixed Checklist
Silent Failure Analysis🟡 **MEDIUM** — fire-and-forget write whose failure is never surfaced (`esphome_device_builder/controllers/config/_preferences_store.py:118-128`)Risk: Fix: Surface debounced-save failures (e.g. log at error level from the Store write path or expose a write-error signal) so a persistent disk failure isn't invisible to operators. 🟡 **MEDIUM** — silent no-op write returning success (`esphome_device_builder/controllers/config/_preferences_store.py:104-117`)Risk: When Fix: When persistence is disabled, propagate a typed error (or flag) from Automated review by Kōan (Claude) |
esphbot
left a comment
There was a problem hiding this comment.
No blocking issues found.
What does this implement/fix?
Splits the preferences plumbing out of the onboarding experience-levels work so it can land on its own. Preferences now ride the
subscribe_eventsinitial_statesnapshot instead of a separateconfig/get_preferencesround trip, served from a RAM-canonicalPreferencesStorethat loads once at startup, debounces writes to its own.device-builder-preferences.json, and migrates the legacy_preferencessidecar blob on first run. A corrupt dedicated file is preserved (renamed to.corrupt) rather than destroyed, and an undecodable legacy blob is left in place.There is no experience-level behaviour here; the onboarding controller just reads the same
onboarding_completed_versionthrough the store now. The experience-levels feature lands on top in a follow up.Related issue or feature (if applicable):
Types of changes
bugfixnew-featureenhancementbreaking-changerefactordocsmaintenancecidependenciesFrontend coordination
Checklist
ruff,codespell, yaml/json/python checks).tests/where applicable.components.index.json/definitions/components/*.jsonhave not been hand-edited (regenerate viascript/sync_components.pyif a sync is needed).docs/ARCHITECTURE.mdand/ordocs/API.md.