Skip to content

Deliver user preferences in the subscribe_events initial_state snapshot#1479

Merged
bdraco merged 6 commits into
mainfrom
preferences-initial-state
Jun 15, 2026
Merged

Deliver user preferences in the subscribe_events initial_state snapshot#1479
bdraco merged 6 commits into
mainfrom
preferences-initial-state

Conversation

@bdraco

@bdraco bdraco commented Jun 15, 2026

Copy link
Copy Markdown
Member

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_events initial_state snapshot instead of a separate config/get_preferences round trip, served from a RAM-canonical PreferencesStore that loads once at startup, debounces writes to its own .device-builder-preferences.json, and migrates the legacy _preferences sidecar 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_version through the store now. The experience-levels feature lands on top in a follow up.

Related issue or feature (if applicable):

  • N/A

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

Checklist

  • The code change is tested and works locally.
  • Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
  • Tests have been added or updated under tests/ where applicable.
  • components.index.json / definitions/components/*.json have not been hand-edited (regenerate via script/sync_components.py if a sync is needed).
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

@github-actions github-actions Bot added the enhancement Improvement to an existing feature label Jun 15, 2026
@bdraco bdraco marked this pull request as ready for review June 15, 2026 13:36
Copilot AI review requested due to automatic review settings June 15, 2026 13:36
@codspeed-hq

codspeed-hq Bot commented Jun 15, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks


Comparing preferences-initial-state (f0a948e) with main (7ee0b75)

Open in CodSpeed

@codecov

codecov Bot commented Jun 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.50%. Comparing base (7ee0b75) to head (f0a948e).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1479   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files         221      222    +1     
  Lines       17166    17253   +87     
=======================================
+ Hits        17081    17168   +87     
  Misses         85       85           
Flag Coverage Δ
py3.12 99.47% <100.00%> (+<0.01%) ⬆️
py3.14 99.50% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...home_device_builder/controllers/config/__init__.py 100.00% <ø> (ø)
...e_builder/controllers/config/_preferences_store.py 100.00% <100.00%> (ø)
...me_device_builder/controllers/config/controller.py 100.00% <100.00%> (ø)
...e_device_builder/controllers/config/preferences.py 100.00% <ø> (ø)
esphome_device_builder/controllers/onboarding.py 100.00% <100.00%> (ø)
esphome_device_builder/device_builder.py 97.75% <100.00%> (+0.02%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 _preferences sidecar key and corrupt-file preservation behavior.
  • Include preferences in subscribe_events initial_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.

Comment thread esphome_device_builder/device_builder.py Outdated
Comment thread esphome_device_builder/controllers/onboarding.py
Comment thread esphome_device_builder/controllers/onboarding.py
Comment thread esphome_device_builder/controllers/config/_preferences_store.py
Comment thread tests/test_subscribe_events_cleanup.py Outdated
@bdraco bdraco marked this pull request as ready for review June 15, 2026 13:43
@esphbot

esphbot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@bdraco

bdraco commented Jun 15, 2026

Copy link
Copy Markdown
Member Author

Addressed the strip-abort suggestion in 84ae3bb: _confirm_and_strip_shared_sync now wraps the metadata_transaction in try/except OSError, logs a warning, and treats the migration as complete rather than aborting boot; the dedicated file is already canonical by then and the leftover legacy key is benign (the dedicated file wins on the next load).

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 .corrupt filename suggestions as-is per the inline replies.

@bdraco

bdraco commented Jun 15, 2026

Copy link
Copy Markdown
Member Author

Following up on the Silent Failure Analysis (missed it in my last reply):

  • MEDIUM (silent field omission, device_builder.py): fixed in de4cb44; the if self.config is not None guard is now if self.config is None: raise RuntimeError(...) then an unconditional assign, so preferences is always present per the docs and a broken invariant fails loud instead of being dropped.
  • MEDIUM (silent success when _persist_disabled, _schedule_save): leaving as the documented tradeoff and out of scope for this extraction. It only trips on a double disk failure (an undecodable dedicated file and a failed .corrupt rename), the disable is already logged at WARNING, and the change still applies in RAM for the session. Threading a "not persisted" signal through the WS command and the frontend for a state that needs two coincident disk failures isn't worth it here; a store-health event would be the right shape if we ever want it.

@esphbot

esphbot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

PR Review — Deliver user preferences in the subscribe_events initial_state snapshot

Clean snapshot-plus-events extraction with genuinely crash-safe migration. Merge-ready.

What's solid:

  • Migration ordering is correct for crash-safety: write the dedicated file → confirm it exists via Store.path.exists() → only then strip the legacy _PREFS_KEY. A swallowed Store write (errors are caught in _async_handle_write) leaves the file absent, so _confirm_and_strip_shared_sync returns False and the legacy blob survives for retry. No window where prefs are lost.
  • Corrupt-data handling preserves rather than destroys: undecodable dedicated file renamed to .corrupt (then sidecar still tried), undecodable legacy blob left in place, both logged before falling back to defaults. Rename failure correctly hard-disables writes so the recoverable file can't be clobbered.
  • snapshot() returns a defensive copy and mutate()/update() always replace RAM (never mutate in place), so a borrowed snapshot reference stays stable — matches the documented invariant and avoids skipping the debounced write.
  • The subscribe_events field is now unconditional with a fail-loud RuntimeError guard (de4cb44), so the wire contract in API.md (preferences always present) actually holds. Docs and the companion frontend PR are linked.

Nothing needs attention to merge. The remaining accepted tradeoffs (silent success when persistence is disabled, fixed .corrupt filename, the sub-second debounce window on mark_acknowledged) are reasonable for a beta feature behind two coincident-disk-failure / sub-second-crash conditions and were each explicitly reasoned through in the thread.



Checklist

  • Migration / backward-compat strategy for state-format change
  • Crash-safety: no preference loss on partial migration
  • Corrupt-input preservation, not destruction
  • Resource flushed on shutdown (config.stop wired)
  • No unsafe deserialization at boundaries
  • No orphaned dead code after helper removal
  • Public API / docs kept in sync
  • Wire contract matches docs (preferences always present)

Silent Failure Analysis

🟡 **MEDIUM** — fire-and-forget write whose failure is never surfaced (`esphome_device_builder/controllers/config/_preferences_store.py:118-128`)

Risk: update/mutate return the new preferences to the client as if persisted, but the actual disk write is a debounced background save whose errors the docstring itself notes the Store swallows, so a disk-full / permission failure silently loses the change with the client told it succeeded.

def _schedule_save(self, *, delay: float) -> None:
    if self._persist_disabled:
        return
    self._store.async_delay_save(self._snapshot, delay=delay)

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 _persist_disabled is set (corrupt file couldn't be renamed aside), _schedule_save returns immediately, so every subsequent set_preferences/onboarding mutation updates only RAM, is lost on restart, yet still returns the updated prefs to the client as though saved — no error reaches the caller.

self._state = UserPreferences.from_dict({**self._state.to_dict(), **fields})
self._schedule_save(delay=delay)
return self._copy()

Fix: When persistence is disabled, propagate a typed error (or flag) from update/mutate so the client/UI knows writes are not durable instead of silently accepting them.


Automated review by Kōan (Claude) HEAD=f0a948e 3 min 44s

@esphbot esphbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@bdraco bdraco merged commit d6054e3 into main Jun 15, 2026
19 checks passed
@bdraco bdraco deleted the preferences-initial-state branch June 15, 2026 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants