From 1114fa06bdc66adb33274af134bdc897dee47b83 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 10:34:56 -0500 Subject: [PATCH 01/20] Add experience-level and remote-compute onboarding preferences Introduces two persistent, user-editable preferences that the onboarding redesign drives off of: - experience_level (beginner / ui / yaml), nullable until chosen - remote_compute_only The onboarding step list is now environment- and preference-aware: the use-case step appears only on non-HA installs and the Wi-Fi step is dropped when remote_compute_only is set. A startup migration defaults pre-existing installs (prior onboarding completed, or device YAML already on disk) to the yaml experience and acknowledges onboarding so the wizard never auto-pops for them. --- docs/API.md | 6 +- .../controllers/onboarding.py | 108 ++++++++++---- esphome_device_builder/device_builder.py | 3 + esphome_device_builder/models/onboarding.py | 4 +- esphome_device_builder/models/preferences.py | 20 +++ tests/test_onboarding_controller.py | 135 ++++++++++++++++-- 6 files changed, 236 insertions(+), 40 deletions(-) diff --git a/docs/API.md b/docs/API.md index a169b15f7..9c4bcab89 100644 --- a/docs/API.md +++ b/docs/API.md @@ -280,7 +280,7 @@ Parsing and writing live on the backend: the frontend exchanges structured `Auto | `config/version` | — | `{server_version, esphome_version}` | Get versions | | `config/serial_ports` | — | `[{port, desc}]` | List serial ports | | `config/get_preferences` | — | `UserPreferences` | Get user preferences | -| `config/set_preferences` | `{theme?, dashboard_view?, ...}` | `UserPreferences` | Update preferences (partial) | +| `config/set_preferences` | `{theme?, dashboard_view?, experience_level?, remote_compute_only?, ...}` | `UserPreferences` | Update preferences (partial). `experience_level` is `beginner` / `ui` / `yaml` (or `null` until chosen); `remote_compute_only` marks an install as a remote build node. | | `config/get_secrets` | — | `[string]` | List secret key names | | `config/set_secret` | `{key, value, overwrite?}` | `{created}` | Atomically set one secret in secrets.yaml under a write lock; `overwrite=false` is create-if-absent | @@ -290,11 +290,11 @@ Parsing and writing live on the backend: the frontend exchanges structured `Auto > > Models: [`OnboardingState`, `OnboardingStep`, `OnboardingStepId`, `OnboardingStepStatus`](../esphome_device_builder/models/onboarding.py) -First-run setup tracking. Each step's `status` is computed from live data on every `get_state` call (never persisted), so the frontend's "needs attention" indicators clear the moment the user fixes the underlying state — even via a manual `secrets.yaml` edit. `completed_version` is the last onboarding-flow version the user has explicitly acknowledged; bumping `ONBOARDING_VERSION` (server-side constant) re-prompts users at lower versions when new steps are added. +First-run setup tracking. Each step's `status` is computed from live data on every `get_state` call (never persisted), so the frontend's "needs attention" indicators clear the moment the user fixes the underlying state — even via a manual `secrets.yaml` edit. `completed_version` is the last onboarding-flow version the user has explicitly acknowledged; bumping `ONBOARDING_VERSION` (server-side constant) re-prompts users at lower versions when new steps are added. The step list is environment- and preference-aware: `use_case` only on non-HA installs, `wifi_credentials` only when not `remote_compute_only`. A pre-existing install (prior onboarding completed, or device YAML already on disk) is migrated to the `yaml` experience at startup so the wizard never auto-pops for it. | Command | Args | Response | Description | |---------|------|----------|-------------| -| `onboarding/get_state` | — | `OnboardingState` | Snapshot of current vs acknowledged version + per-step `pending` / `done` status. Currently one step (`wifi_credentials`) — pending when `secrets.yaml`'s `wifi_ssid` is missing, empty, whitespace-only, or matches the bootstrap placeholder. | +| `onboarding/get_state` | — | `OnboardingState` | Snapshot of current vs acknowledged version + per-step `pending` / `done` status. Steps: `use_case` (non-HA only — remote-compute choice) and `experience_level` track whether `experience_level` is set; `wifi_credentials` (omitted when `remote_compute_only`) is pending when `secrets.yaml`'s `wifi_ssid` is missing, empty, whitespace-only, or matches the bootstrap placeholder. | | `onboarding/set_wifi_credentials` | `{ssid, password?}` | `OnboardingState` | Update `wifi_ssid` / `wifi_password` in `secrets.yaml` via a line-based rewrite that preserves standalone and inline trailing comments and other secrets. Validates against ESPHome's own length limits (32 char SSID, 64 char password) plus a control-character check; empty / whitespace-only SSID, oversize values, and control characters (other than `\t`) raise `INVALID_ARGS`. `password` is optional and defaults to the empty string for open networks. | | `onboarding/mark_acknowledged` | — | `OnboardingState` | Record that the user has finished the current onboarding flow (sets `onboarding_completed_version` to `ONBOARDING_VERSION`). Idempotent and monotonic — never downgrades a higher stored value. Use this on save AND on explicit decline ("I don't use Wi-Fi") so the wizard stops re-popping; the per-step `pending` status stays accurate so the dedicated `Set up Wi-Fi…` kebab entry still surfaces the re-entry path until the underlying data is set. | diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index c0322527c..013ecb1e4 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -20,6 +20,7 @@ from __future__ import annotations import asyncio +from functools import partial from pathlib import Path from typing import TYPE_CHECKING, Any @@ -31,6 +32,7 @@ ) from ..models import ( ErrorCode, + ExperienceLevel, OnboardingState, OnboardingStep, OnboardingStepId, @@ -39,6 +41,7 @@ ) from ..models.onboarding import ONBOARDING_VERSION from .config import load_preferences, mutate_preferences +from .config.settings import _DASHBOARD_SENTINEL_FILE if TYPE_CHECKING: from esphome_device_builder.device_builder import DeviceBuilder @@ -63,28 +66,30 @@ async def get_state(self, **kwargs: Any) -> OnboardingState: """ Return the current onboarding snapshot. - Computes each step's status from live data, then reads the - user's last-acknowledged version from preferences. The - frontend combines the two to decide whether to surface the - wizard (any pending step OR new version available). + The step list is environment- and preference-aware: the + use-case step only on non-HA installs, the Wi-Fi step only + when not remote-compute-only. Each status is computed from + live data; the frontend surfaces the wizard on any pending + step or a newer version. """ loop = asyncio.get_running_loop() - secrets, prefs = await loop.run_in_executor( - None, _read_secrets_and_prefs, self._db.settings.config_dir + settings = self._db.settings + return await loop.run_in_executor( + None, + partial(_compute_state, settings.config_dir, on_ha_addon=settings.on_ha_addon), ) - return OnboardingState( - current_version=ONBOARDING_VERSION, - completed_version=prefs.onboarding_completed_version, - steps=[ - OnboardingStep( - id=OnboardingStepId.WIFI_CREDENTIALS, - status=OnboardingStepStatus.PENDING - if is_wifi_unconfigured(secrets) - else OnboardingStepStatus.DONE, - ), - ], - ) + async def migrate_preexisting_install(self) -> None: + """ + Default a pre-existing install to the YAML experience, once. + + Installs that completed an earlier onboarding or already hold + device YAMLs predate the experience picker; mark them YAML + users and acknowledge onboarding so the wizard never auto-pops. + Idempotent — a no-op once ``experience_level`` is set. + """ + loop = asyncio.get_running_loop() + await loop.run_in_executor(None, _migrate_preexisting, self._db.settings.config_dir) @api_command("onboarding/set_wifi_credentials") async def set_wifi_credentials( @@ -175,12 +180,67 @@ def _bump(prefs: UserPreferences) -> None: return await self.get_state() -def _read_secrets_and_prefs(config_dir: Path) -> tuple[dict | None, UserPreferences]: +# YAML files in the config dir that aren't user device configs. +_NON_DEVICE_YAML = frozenset({"secrets.yaml", _DASHBOARD_SENTINEL_FILE}) + + +def _compute_state(config_dir: Path, *, on_ha_addon: bool) -> OnboardingState: """ - Read both ``secrets.yaml`` and user preferences in one executor hop. + Build the onboarding snapshot in one executor hop. - Both are quick disk reads from the same config dir, so a single - executor job is cheaper than two. ``get_state`` runs on every - page load + after every secrets save, so the saved hop matters. + Reads ``secrets.yaml`` + preferences (both quick reads from the + same dir) and assembles the environment-aware step list. """ - return read_secrets_yaml(config_dir), load_preferences(config_dir) + secrets = read_secrets_yaml(config_dir) + prefs = load_preferences(config_dir) + experience_done = _status(done=prefs.experience_level is not None) + + steps: list[OnboardingStep] = [] + # Use-case (remote-compute?) is a non-HA question; HA users manage + # devices in Home Assistant. Its status tracks the experience pick, + # which the wizard always answers in the same pass. + if not on_ha_addon: + steps.append(OnboardingStep(id=OnboardingStepId.USE_CASE, status=experience_done)) + steps.append(OnboardingStep(id=OnboardingStepId.EXPERIENCE_LEVEL, status=experience_done)) + if not prefs.remote_compute_only: + steps.append( + OnboardingStep( + id=OnboardingStepId.WIFI_CREDENTIALS, + status=_status(done=not is_wifi_unconfigured(secrets)), + ) + ) + + return OnboardingState( + current_version=ONBOARDING_VERSION, + completed_version=prefs.onboarding_completed_version, + steps=steps, + ) + + +def _migrate_preexisting(config_dir: Path) -> None: + """Mark a pre-existing install YAML + acknowledged; no-op otherwise.""" + prefs = load_preferences(config_dir) + if prefs.experience_level is not None: + return + if prefs.onboarding_completed_version == 0 and not _has_device_configs(config_dir): + return + + def _mark(p: UserPreferences) -> None: + p.experience_level = ExperienceLevel.YAML + # max(), not assign: never downgrade a higher stored value. + p.onboarding_completed_version = max(p.onboarding_completed_version, ONBOARDING_VERSION) + + mutate_preferences(config_dir, _mark) + + +def _has_device_configs(config_dir: Path) -> bool: + """Return True when the config dir holds any user device YAML.""" + try: + return any(p.name not in _NON_DEVICE_YAML for p in config_dir.glob("*.yaml")) + except OSError: + return False + + +def _status(*, done: bool) -> OnboardingStepStatus: + """Map a done-ness boolean to the step status enum.""" + return OnboardingStepStatus.DONE if done else OnboardingStepStatus.PENDING diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index ba5b07e8e..9399b59a7 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -336,6 +336,9 @@ async def start(self) -> None: self.remote_build_offloader = OffloaderController(self) self.remote_build_receiver = ReceiverController(self) self.version_history = VersionHistoryController(self) + # Default pre-existing installs to the YAML experience before + # any onboarding command can be served. + await self.onboarding.migrate_preexisting_install() await self.devices.start() await self.firmware.start() await self.editor.start() diff --git a/esphome_device_builder/models/onboarding.py b/esphome_device_builder/models/onboarding.py index b144e799c..f9cc63c72 100644 --- a/esphome_device_builder/models/onboarding.py +++ b/esphome_device_builder/models/onboarding.py @@ -37,6 +37,8 @@ class OnboardingStepId(StrEnum): the wire-format string the frontend dispatches on. """ + USE_CASE = "use_case" + EXPERIENCE_LEVEL = "experience_level" WIFI_CREDENTIALS = "wifi_credentials" @@ -65,7 +67,7 @@ class OnboardingStep(DataClassORJSONMixin): # already at the current version doesn't get re-prompted unless # a step is data-derived-pending (e.g. they manually deleted # ``wifi_ssid`` from ``secrets.yaml``). -ONBOARDING_VERSION: int = 1 +ONBOARDING_VERSION: int = 2 @dataclass diff --git a/esphome_device_builder/models/preferences.py b/esphome_device_builder/models/preferences.py index c0be862cc..3ef4653ff 100644 --- a/esphome_device_builder/models/preferences.py +++ b/esphome_device_builder/models/preferences.py @@ -30,6 +30,19 @@ class SortDirection(StrEnum): DESC = "desc" +class ExperienceLevel(StrEnum): + """How much ESPHome the user knows — tailors UI weight. + + Chosen in onboarding, changeable any time in Settings. ``None`` + (a fresh install that hasn't picked) is handled separately; a + pre-existing install migrates to ``YAML``. + """ + + BEGINNER = "beginner" + UI = "ui" + YAML = "yaml" + + @dataclass class UserPreferences(DataClassORJSONMixin): """Per-user UI preferences. @@ -52,6 +65,13 @@ class UserPreferences(DataClassORJSONMixin): table_sort_column: str | None = None table_sort_direction: SortDirection | None = None + # Experience level chosen in onboarding (None = not yet chosen). + # Seeds ``yaml_diff_button`` and the editor's first-open layout. + experience_level: ExperienceLevel | None = None + # This install is only a remote build node: onboarding skips the + # Wi-Fi step and device-creation entry points are hidden. + remote_compute_only: bool = False + # Highest onboarding-flow version the user has acknowledged. # Default 0 ⇒ never gone through onboarding; the dashboard # surfaces the wizard on next load. See diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index 767919186..77a44d832 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -31,24 +31,35 @@ ) from esphome_device_builder.models.onboarding import ( ONBOARDING_VERSION, + OnboardingState, OnboardingStepId, OnboardingStepStatus, ) -from esphome_device_builder.models.preferences import Theme, UserPreferences +from esphome_device_builder.models.preferences import ( + ExperienceLevel, + Theme, + UserPreferences, +) from .conftest import wire_secrets_writer -def _make_controller(config_dir: Path) -> OnboardingController: +def _make_controller(config_dir: Path, *, on_ha_addon: bool = False) -> OnboardingController: controller = OnboardingController.__new__(OnboardingController) controller._db = MagicMock() controller._db.settings.config_dir = config_dir controller._db.settings.absolute_config_dir = config_dir.resolve() + controller._db.settings.on_ha_addon = on_ha_addon controller._db.secrets_write_lock = asyncio.Lock() wire_secrets_writer(controller._db) return controller +def _step(state: OnboardingState, step_id: OnboardingStepId) -> OnboardingStepStatus: + """Status of one step by id, or None when the step isn't in the list.""" + return next((s.status for s in state.steps if s.id == step_id), None) + + def _write_secrets(config_dir: Path, content: str) -> None: (config_dir / "secrets.yaml").write_text(content) @@ -64,9 +75,7 @@ async def test_get_state_pending_for_missing_secrets(tmp_path: Path) -> None: state = await controller.get_state() assert state.current_version == ONBOARDING_VERSION assert state.completed_version == 0 - assert len(state.steps) == 1 - assert state.steps[0].id == OnboardingStepId.WIFI_CREDENTIALS - assert state.steps[0].status == OnboardingStepStatus.PENDING + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.PENDING async def test_get_state_pending_for_empty_string_secrets(tmp_path: Path) -> None: @@ -74,7 +83,7 @@ async def test_get_state_pending_for_empty_string_secrets(tmp_path: Path) -> Non _write_secrets(tmp_path, 'wifi_ssid: ""\nwifi_password: ""\n') controller = _make_controller(tmp_path) state = await controller.get_state() - assert state.steps[0].status == OnboardingStepStatus.PENDING + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.PENDING async def test_get_state_pending_for_placeholder_secrets(tmp_path: Path) -> None: @@ -85,14 +94,61 @@ async def test_get_state_pending_for_placeholder_secrets(tmp_path: Path) -> None ) controller = _make_controller(tmp_path) state = await controller.get_state() - assert state.steps[0].status == OnboardingStepStatus.PENDING + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.PENDING async def test_get_state_done_for_real_secrets(tmp_path: Path) -> None: _write_secrets(tmp_path, "wifi_ssid: home_network\nwifi_password: hunter2\n") controller = _make_controller(tmp_path) state = await controller.get_state() - assert state.steps[0].status == OnboardingStepStatus.DONE + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.DONE + + +# --------------------------------------------------------------------------- +# get_state — environment- and preference-aware step list +# --------------------------------------------------------------------------- + + +async def test_get_state_non_ha_includes_use_case_step(tmp_path: Path) -> None: + """Non-HA installs ask the remote-compute use-case question.""" + controller = _make_controller(tmp_path, on_ha_addon=False) + state = await controller.get_state() + ids = [s.id for s in state.steps] + assert ids == [ + OnboardingStepId.USE_CASE, + OnboardingStepId.EXPERIENCE_LEVEL, + OnboardingStepId.WIFI_CREDENTIALS, + ] + assert _step(state, OnboardingStepId.USE_CASE) == OnboardingStepStatus.PENDING + assert _step(state, OnboardingStepId.EXPERIENCE_LEVEL) == OnboardingStepStatus.PENDING + + +async def test_get_state_ha_addon_omits_use_case_step(tmp_path: Path) -> None: + """HA addon manages devices in HA, so no use-case question.""" + controller = _make_controller(tmp_path, on_ha_addon=True) + state = await controller.get_state() + ids = [s.id for s in state.steps] + assert ids == [OnboardingStepId.EXPERIENCE_LEVEL, OnboardingStepId.WIFI_CREDENTIALS] + + +async def test_get_state_experience_set_marks_use_case_and_experience_done( + tmp_path: Path, +) -> None: + """Picking an experience level completes both leading steps.""" + await asyncio.to_thread(update_preferences, tmp_path, {"experience_level": ExperienceLevel.UI}) + controller = _make_controller(tmp_path, on_ha_addon=False) + state = await controller.get_state() + assert _step(state, OnboardingStepId.USE_CASE) == OnboardingStepStatus.DONE + assert _step(state, OnboardingStepId.EXPERIENCE_LEVEL) == OnboardingStepStatus.DONE + + +async def test_get_state_remote_compute_only_drops_wifi_step(tmp_path: Path) -> None: + """A remote-compute-only install skips Wi-Fi setup entirely.""" + await asyncio.to_thread(update_preferences, tmp_path, {"remote_compute_only": True}) + controller = _make_controller(tmp_path, on_ha_addon=False) + state = await controller.get_state() + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) is None + assert OnboardingStepId.USE_CASE in [s.id for s in state.steps] # --------------------------------------------------------------------------- @@ -108,7 +164,7 @@ async def test_set_wifi_credentials_writes_to_secrets_yaml(tmp_path: Path) -> No ) controller = _make_controller(tmp_path) state = await controller.set_wifi_credentials(ssid="home_network", password="hunter2") - assert state.steps[0].status == OnboardingStepStatus.DONE + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.DONE content = (tmp_path / "secrets.yaml").read_text() assert 'wifi_ssid: "home_network"' in content assert 'wifi_password: "hunter2"' in content @@ -212,7 +268,7 @@ async def test_set_wifi_credentials_accepts_empty_password(tmp_path: Path) -> No """Open networks have empty passwords — must not be rejected.""" controller = _make_controller(tmp_path) state = await controller.set_wifi_credentials(ssid="OpenNet", password="") - assert state.steps[0].status == OnboardingStepStatus.DONE + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.DONE # --------------------------------------------------------------------------- @@ -324,7 +380,7 @@ async def test_set_wifi_credentials_allows_tab_in_value(tmp_path: Path) -> None: """ controller = _make_controller(tmp_path) state = await controller.set_wifi_credentials(ssid="MyAP", password="hunter\t2") - assert state.steps[0].status == OnboardingStepStatus.DONE + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.DONE async def test_set_wifi_credentials_preserves_inline_comments( @@ -390,7 +446,7 @@ async def test_get_state_pending_for_malformed_secrets_yaml(tmp_path: Path) -> N _write_secrets(tmp_path, "wifi_ssid: [unclosed\n") controller = _make_controller(tmp_path) state = await controller.get_state() - assert state.steps[0].status == OnboardingStepStatus.PENDING + assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) == OnboardingStepStatus.PENDING # --------------------------------------------------------------------------- @@ -498,6 +554,61 @@ def test_replace_or_append_secret_value_with_hash_in_quotes_is_misparsed() -> No assert result == 'wifi_ssid: "MyAP" # bar"\n' +# --------------------------------------------------------------------------- +# migrate_preexisting_install +# --------------------------------------------------------------------------- + + +async def test_migrate_preexisting_acknowledged_install_becomes_yaml(tmp_path: Path) -> None: + """An install that completed an earlier onboarding defaults to YAML.""" + await asyncio.to_thread( + save_preferences, tmp_path, UserPreferences(onboarding_completed_version=1) + ) + await _make_controller(tmp_path).migrate_preexisting_install() + prefs = await asyncio.to_thread(load_preferences, tmp_path) + assert prefs.experience_level == ExperienceLevel.YAML + assert prefs.onboarding_completed_version == ONBOARDING_VERSION + + +async def test_migrate_preexisting_install_with_device_yaml_becomes_yaml( + tmp_path: Path, +) -> None: + """A config dir already holding device YAML predates the picker ⇒ YAML.""" + (tmp_path / "living-room.yaml").write_text("esphome:\n name: living-room\n") + await _make_controller(tmp_path).migrate_preexisting_install() + prefs = await asyncio.to_thread(load_preferences, tmp_path) + assert prefs.experience_level == ExperienceLevel.YAML + + +async def test_migrate_fresh_install_is_noop(tmp_path: Path) -> None: + """No prior onboarding and no device YAML ⇒ stay unchosen, see the wizard.""" + await _make_controller(tmp_path).migrate_preexisting_install() + prefs = await asyncio.to_thread(load_preferences, tmp_path) + assert prefs.experience_level is None + assert prefs.onboarding_completed_version == 0 + + +async def test_migrate_ignores_secrets_yaml(tmp_path: Path) -> None: + """``secrets.yaml`` alone is not a device config — no migration.""" + _write_secrets(tmp_path, "wifi_ssid: home\n") + await _make_controller(tmp_path).migrate_preexisting_install() + prefs = await asyncio.to_thread(load_preferences, tmp_path) + assert prefs.experience_level is None + + +async def test_migrate_preserves_an_explicit_choice(tmp_path: Path) -> None: + """A user who already picked BEGINNER isn't overwritten by migration.""" + await asyncio.to_thread( + save_preferences, + tmp_path, + UserPreferences(experience_level=ExperienceLevel.BEGINNER, onboarding_completed_version=2), + ) + (tmp_path / "device.yaml").write_text("esphome:\n name: device\n") + await _make_controller(tmp_path).migrate_preexisting_install() + prefs = await asyncio.to_thread(load_preferences, tmp_path) + assert prefs.experience_level == ExperienceLevel.BEGINNER + + # --------------------------------------------------------------------------- # Constructor smoke # --------------------------------------------------------------------------- From aeb219f34ca76dc9c94698de2c67ec13da211c98 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 11:31:08 -0500 Subject: [PATCH 02/20] Use canonical yaml-file discovery in onboarding migration The migration's config detection globbed only *.yaml, so a .yml-only install that never acknowledged onboarding was misclassified as fresh and got the wizard popped, the opposite of the migration's intent. Switch to esphome.util.list_yaml_files so the extension, secrets, and dotfile rules match the device scanner and can't drift. Log the scan OSError instead of silently swallowing it, and add .yml + unreadable-dir tests. --- .../controllers/onboarding.py | 26 ++++++++++++++----- tests/test_onboarding_controller.py | 16 ++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index 013ecb1e4..3c9cc2b0f 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -20,10 +20,13 @@ from __future__ import annotations import asyncio +import logging from functools import partial from pathlib import Path from typing import TYPE_CHECKING, Any +from esphome.util import list_yaml_files + from ..helpers.api import CommandError, api_command from ..helpers.secrets_state import ( is_wifi_unconfigured, @@ -46,6 +49,8 @@ if TYPE_CHECKING: from esphome_device_builder.device_builder import DeviceBuilder +_LOGGER = logging.getLogger(__name__) + # Cap inputs at the same length ESPHome's own validators enforce — # ``cv.ssid`` (32 chars) and the WPA password validator (64 chars). @@ -180,10 +185,6 @@ def _bump(prefs: UserPreferences) -> None: return await self.get_state() -# YAML files in the config dir that aren't user device configs. -_NON_DEVICE_YAML = frozenset({"secrets.yaml", _DASHBOARD_SENTINEL_FILE}) - - def _compute_state(config_dir: Path, *, on_ha_addon: bool) -> OnboardingState: """ Build the onboarding snapshot in one executor hop. @@ -234,10 +235,23 @@ def _mark(p: UserPreferences) -> None: def _has_device_configs(config_dir: Path) -> bool: - """Return True when the config dir holds any user device YAML.""" + """ + Return True when the config dir holds any user device YAML. + + Uses the canonical ``list_yaml_files`` rule (.yaml + .yml, secrets + and dotfiles excluded) so it can't drift from the device scanner; + only the dashboard sentinel needs excluding on top. + """ try: - return any(p.name not in _NON_DEVICE_YAML for p in config_dir.glob("*.yaml")) + return any(p.name != _DASHBOARD_SENTINEL_FILE for p in list_yaml_files([config_dir])) except OSError: + # A read failure must not silently reclassify a real install as + # fresh (which would pop the wizard); log it and treat as empty. + _LOGGER.warning( + "Could not scan %s for device configs during onboarding migration", + config_dir, + exc_info=True, + ) return False diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index 77a44d832..5a7e1a9ed 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -22,6 +22,7 @@ ) from esphome_device_builder.controllers.onboarding import ( OnboardingController, + _has_device_configs, ) from esphome_device_builder.helpers.api import CommandError from esphome_device_builder.helpers.secrets_state import ( @@ -580,6 +581,21 @@ async def test_migrate_preexisting_install_with_device_yaml_becomes_yaml( assert prefs.experience_level == ExperienceLevel.YAML +async def test_migrate_preexisting_install_with_yml_extension_becomes_yaml( + tmp_path: Path, +) -> None: + """``.yml`` is an equally valid config extension; it must trigger migration too.""" + (tmp_path / "bedroom.yml").write_text("esphome:\n name: bedroom\n") + await _make_controller(tmp_path).migrate_preexisting_install() + prefs = await asyncio.to_thread(load_preferences, tmp_path) + assert prefs.experience_level == ExperienceLevel.YAML + + +def test_has_device_configs_unreadable_dir_returns_false(tmp_path: Path) -> None: + """A scan failure soft-fails to ``False`` rather than crashing startup.""" + assert _has_device_configs(tmp_path / "does-not-exist") is False + + async def test_migrate_fresh_install_is_noop(tmp_path: Path) -> None: """No prior onboarding and no device YAML ⇒ stay unchosen, see the wizard.""" await _make_controller(tmp_path).migrate_preexisting_install() From 180054617142bc76dd012a2ef01a4dee935b0497 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 11:42:42 -0500 Subject: [PATCH 03/20] Fail safe for existing users on config-scan read error A missing config dir is a genuinely fresh install (no configs, return False). A dir that exists but can't be read now returns True instead, so a transient read error can't reclassify a real install as fresh and re-pop the wizard, which is the failure direction the migration is meant to avoid. FileNotFoundError keeps the fresh-install path; other OSError assumes pre-existing and logs. --- .../controllers/onboarding.py | 13 +++++++++---- tests/test_onboarding_controller.py | 19 ++++++++++++++++--- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index 3c9cc2b0f..db255307a 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -241,18 +241,23 @@ def _has_device_configs(config_dir: Path) -> bool: Uses the canonical ``list_yaml_files`` rule (.yaml + .yml, secrets and dotfiles excluded) so it can't drift from the device scanner; only the dashboard sentinel needs excluding on top. + + A missing dir is a genuinely fresh install (return False). A dir that + exists but can't be read fails *safe for existing users*: assume it + holds configs so a transient read error can't reclassify a real + install as fresh and re-pop the wizard. """ try: return any(p.name != _DASHBOARD_SENTINEL_FILE for p in list_yaml_files([config_dir])) + except FileNotFoundError: + return False except OSError: - # A read failure must not silently reclassify a real install as - # fresh (which would pop the wizard); log it and treat as empty. _LOGGER.warning( - "Could not scan %s for device configs during onboarding migration", + "Could not scan %s for device configs; assuming pre-existing install", config_dir, exc_info=True, ) - return False + return True def _status(*, done: bool) -> OnboardingStepStatus: diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index 5a7e1a9ed..f5ec4cfa3 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -11,7 +11,7 @@ import asyncio from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch import pytest @@ -591,11 +591,24 @@ async def test_migrate_preexisting_install_with_yml_extension_becomes_yaml( assert prefs.experience_level == ExperienceLevel.YAML -def test_has_device_configs_unreadable_dir_returns_false(tmp_path: Path) -> None: - """A scan failure soft-fails to ``False`` rather than crashing startup.""" +def test_has_device_configs_missing_dir_returns_false(tmp_path: Path) -> None: + """A genuinely-absent config dir is a fresh install, not a scan failure.""" assert _has_device_configs(tmp_path / "does-not-exist") is False +def test_has_device_configs_unreadable_dir_assumes_preexisting(tmp_path: Path) -> None: + """A dir that exists but can't be read fails safe for existing users. + + A transient read error must not reclassify a real install as fresh and + re-pop the wizard, so it assumes configs are present. + """ + with patch( + "esphome_device_builder.controllers.onboarding.list_yaml_files", + side_effect=PermissionError("denied"), + ): + assert _has_device_configs(tmp_path) is True + + async def test_migrate_fresh_install_is_noop(tmp_path: Path) -> None: """No prior onboarding and no device YAML ⇒ stay unchosen, see the wizard.""" await _make_controller(tmp_path).migrate_preexisting_install() From e91a3e3697da564eb52d4dafaaec86872930169f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 11:51:56 -0500 Subject: [PATCH 04/20] Hoist shared onboarding-version bump helper mark_acknowledged and the pre-existing migration bumped onboarding_completed_version with the same max() expression and the same comment. Extract _acknowledge_current_version so the monotonic rule lives in one place; mark_acknowledged now passes it straight to mutate_preferences instead of wrapping a closure. --- .../controllers/onboarding.py | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index db255307a..adf519b88 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -173,15 +173,9 @@ async def mark_acknowledged(self, **kwargs: Any) -> OnboardingState: """ loop = asyncio.get_running_loop() config_dir = self._db.settings.config_dir - - def _bump(prefs: UserPreferences) -> None: - # max(), not assign: a rollback from a future build must - # not downgrade a higher stored acknowledgement. - prefs.onboarding_completed_version = max( - prefs.onboarding_completed_version, ONBOARDING_VERSION - ) - - await loop.run_in_executor(None, mutate_preferences, config_dir, _bump) + await loop.run_in_executor( + None, mutate_preferences, config_dir, _acknowledge_current_version + ) return await self.get_state() @@ -228,12 +222,21 @@ def _migrate_preexisting(config_dir: Path) -> None: def _mark(p: UserPreferences) -> None: p.experience_level = ExperienceLevel.YAML - # max(), not assign: never downgrade a higher stored value. - p.onboarding_completed_version = max(p.onboarding_completed_version, ONBOARDING_VERSION) + _acknowledge_current_version(p) mutate_preferences(config_dir, _mark) +def _acknowledge_current_version(prefs: UserPreferences) -> None: + """ + Raise the acknowledged onboarding version to current, never downgrading. + + max(), not assign: a rollback from a future build must not downgrade a + higher stored acknowledgement. + """ + prefs.onboarding_completed_version = max(prefs.onboarding_completed_version, ONBOARDING_VERSION) + + def _has_device_configs(config_dir: Path) -> bool: """ Return True when the config dir holds any user device YAML. From e720453cdbc741650801007cbf78582a6939c2de Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 14:37:37 -0500 Subject: [PATCH 05/20] Address review nits: docstring style and test helper return type - Move the ExperienceLevel multi-line docstring summary to the line after the opening quotes, per the repo docstring convention. - Annotate the _step test helper as OnboardingStepStatus | None to match its next(..., None) behaviour. --- esphome_device_builder/models/preferences.py | 3 ++- tests/test_onboarding_controller.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/esphome_device_builder/models/preferences.py b/esphome_device_builder/models/preferences.py index 3ef4653ff..195086244 100644 --- a/esphome_device_builder/models/preferences.py +++ b/esphome_device_builder/models/preferences.py @@ -31,7 +31,8 @@ class SortDirection(StrEnum): class ExperienceLevel(StrEnum): - """How much ESPHome the user knows — tailors UI weight. + """ + How much ESPHome the user knows; tailors UI weight. Chosen in onboarding, changeable any time in Settings. ``None`` (a fresh install that hasn't picked) is handled separately; a diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index f5ec4cfa3..672fbbbfb 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -56,7 +56,7 @@ def _make_controller(config_dir: Path, *, on_ha_addon: bool = False) -> Onboardi return controller -def _step(state: OnboardingState, step_id: OnboardingStepId) -> OnboardingStepStatus: +def _step(state: OnboardingState, step_id: OnboardingStepId) -> OnboardingStepStatus | None: """Status of one step by id, or None when the step isn't in the list.""" return next((s.status for s in state.steps if s.id == step_id), None) From fc2b56d416caff87eb5f00669e5eba9186fffd1f Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 15:02:06 -0500 Subject: [PATCH 06/20] Keep Wi-Fi onboarding for migrated installs missing Wi-Fi The migration acknowledged onboarding for every pre-existing install, which also silenced the Wi-Fi prompt. Now it only acknowledges installs that already completed an earlier onboarding (their prior Wi-Fi save or decline stands); an install known only by its device YAML is migrated to the YAML experience but left un-acknowledged, so a missing-Wi-Fi prompt still surfaces. --- .../controllers/onboarding.py | 15 +++++++++-- tests/test_onboarding_controller.py | 25 ++++++++++++------- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index adf519b88..33fbd6662 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -213,7 +213,13 @@ def _compute_state(config_dir: Path, *, on_ha_addon: bool) -> OnboardingState: def _migrate_preexisting(config_dir: Path) -> None: - """Mark a pre-existing install YAML + acknowledged; no-op otherwise.""" + """ + Default a pre-existing install to the YAML experience; no-op otherwise. + + Installs that already completed an earlier onboarding are also marked + acknowledged (their prior Wi-Fi choice stands); installs known only by a + device YAML are left un-acknowledged so a missing-Wi-Fi prompt still fires. + """ prefs = load_preferences(config_dir) if prefs.experience_level is not None: return @@ -222,7 +228,12 @@ def _migrate_preexisting(config_dir: Path) -> None: def _mark(p: UserPreferences) -> None: p.experience_level = ExperienceLevel.YAML - _acknowledge_current_version(p) + # Only acknowledge onboarding for installs that already completed it, + # so a prior Wi-Fi save or decline is respected. An install known only + # by its device YAML stays un-acknowledged, so a missing-Wi-Fi prompt + # still surfaces. + if p.onboarding_completed_version > 0: + _acknowledge_current_version(p) mutate_preferences(config_dir, _mark) diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index 672fbbbfb..7ce4dcb6b 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -560,8 +560,13 @@ def test_replace_or_append_secret_value_with_hash_in_quotes_is_misparsed() -> No # --------------------------------------------------------------------------- -async def test_migrate_preexisting_acknowledged_install_becomes_yaml(tmp_path: Path) -> None: - """An install that completed an earlier onboarding defaults to YAML.""" +async def test_migrate_acknowledged_install_becomes_yaml_and_stays_acknowledged( + tmp_path: Path, +) -> None: + """An install that completed an earlier onboarding keeps its acknowledgement. + + Their prior Wi-Fi save / decline stands, so onboarding is bumped to current. + """ await asyncio.to_thread( save_preferences, tmp_path, UserPreferences(onboarding_completed_version=1) ) @@ -571,24 +576,26 @@ async def test_migrate_preexisting_acknowledged_install_becomes_yaml(tmp_path: P assert prefs.onboarding_completed_version == ONBOARDING_VERSION -async def test_migrate_preexisting_install_with_device_yaml_becomes_yaml( - tmp_path: Path, -) -> None: - """A config dir already holding device YAML predates the picker ⇒ YAML.""" +async def test_migrate_device_yaml_install_stays_unacknowledged(tmp_path: Path) -> None: + """A config-only install that never onboarded gets YAML but no acknowledgement. + + Leaving ``onboarding_completed_version`` at 0 lets a missing-Wi-Fi prompt + still fire for these users. + """ (tmp_path / "living-room.yaml").write_text("esphome:\n name: living-room\n") await _make_controller(tmp_path).migrate_preexisting_install() prefs = await asyncio.to_thread(load_preferences, tmp_path) assert prefs.experience_level == ExperienceLevel.YAML + assert prefs.onboarding_completed_version == 0 -async def test_migrate_preexisting_install_with_yml_extension_becomes_yaml( - tmp_path: Path, -) -> None: +async def test_migrate_install_with_yml_extension_becomes_yaml(tmp_path: Path) -> None: """``.yml`` is an equally valid config extension; it must trigger migration too.""" (tmp_path / "bedroom.yml").write_text("esphome:\n name: bedroom\n") await _make_controller(tmp_path).migrate_preexisting_install() prefs = await asyncio.to_thread(load_preferences, tmp_path) assert prefs.experience_level == ExperienceLevel.YAML + assert prefs.onboarding_completed_version == 0 def test_has_device_configs_missing_dir_returns_false(tmp_path: Path) -> None: From 46c93a844debe331717cccdde65c4ef03b9f412a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 20:33:42 -0500 Subject: [PATCH 07/20] Ship user preferences in the subscribe_events initial_state The frontend gates first-paint UI on the experience level and remote-compute flag, so pushing them in the subscription snapshot lets the client paint without a separate config/get_preferences round-trip whose failure would otherwise uniquely break device creation. Read sync in _send_initial per the hot-path rule; the metadata sidecar is small. --- docs/API.md | 2 +- esphome_device_builder/device_builder.py | 6 +++ tests/test_subscribe_events_cleanup.py | 51 +++++++++++++++++++++++- 3 files changed, 57 insertions(+), 2 deletions(-) diff --git a/docs/API.md b/docs/API.md index 9c4bcab89..c7036d594 100644 --- a/docs/API.md +++ b/docs/API.md @@ -426,7 +426,7 @@ Same-subnet peers read `remote_build_port` from TXT so a `--remote-build-port` o **`subscribe_events` initial state:** -Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. Each field is present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync reads, no executor hop, no disk I/O. The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. +Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{preferences: UserPreferences, devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. `preferences` is always present — its `experience_level` and `remote_compute_only` fields gate first-paint UI (which editor surfaces show, whether device-creation entry points are hidden), so they ride the snapshot rather than make the client chase a separate `config/get_preferences`. The rest are present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync reads, no executor hop; the only disk touch is the small `preferences` sidecar (`load_preferences`). The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. **`subscribe_events` events:** - `device_added`, `device_removed`, `device_updated`, `device_state_changed` diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index 9399b59a7..d5ad8a0e3 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -32,6 +32,7 @@ from .controllers.config import ( ConfigController, DashboardSettings, + load_preferences, ) from .controllers.devices import DevicesController from .controllers.editor import EditorController @@ -595,6 +596,11 @@ async def _send_initial(_controls: StreamControls) -> None: # snapshot here a fresh page load would miss everything # the dashboard had already accumulated by then. initial: dict[str, Any] = {} + # Preferences gate first-paint UI (experience level, remote-compute + # creation hiding), so ship them in the snapshot rather than make the + # client chase a separate get_preferences round-trip. Sync read per + # the hot-path rule; the metadata sidecar is small. + initial["preferences"] = load_preferences(self.settings.config_dir).to_dict() if self.devices: initial["devices"] = [d.to_dict() for d in self.devices.get_devices()] initial["importable"] = [d.to_dict() for d in self.devices.get_importable_devices()] diff --git a/tests/test_subscribe_events_cleanup.py b/tests/test_subscribe_events_cleanup.py index 5723c1063..4486fdc91 100644 --- a/tests/test_subscribe_events_cleanup.py +++ b/tests/test_subscribe_events_cleanup.py @@ -14,11 +14,14 @@ from __future__ import annotations import asyncio +import tempfile +from pathlib import Path from typing import Any from unittest.mock import MagicMock import pytest +from esphome_device_builder.controllers.config import save_preferences from esphome_device_builder.device_builder import DeviceBuilder from esphome_device_builder.helpers.event_bus import ( _DEFAULT_STREAM_QUEUE_MAX, @@ -26,10 +29,15 @@ StreamBackpressureError, ) from esphome_device_builder.helpers.subscriber_presence import SubscriberPresence -from esphome_device_builder.models import EventType +from esphome_device_builder.models import EventType, UserPreferences +from esphome_device_builder.models.preferences import ExperienceLevel from .conftest import FakeWebSocketClient +# ``_send_initial`` reads preferences off ``settings.config_dir``; an empty dir +# makes ``load_preferences`` return defaults, which is all these stubs need. +_STUB_CONFIG_DIR = Path(tempfile.mkdtemp()) + def _make_db() -> DeviceBuilder: """Build a minimally-initialised DeviceBuilder for the handler. @@ -40,6 +48,7 @@ def _make_db() -> DeviceBuilder: stub. """ db = DeviceBuilder.__new__(DeviceBuilder) + db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None # skip the device-snapshot branch @@ -143,6 +152,7 @@ async def test_subscribe_events_includes_pairings_snapshot_in_initial_state() -> hop, no disk read. """ db = DeviceBuilder.__new__(DeviceBuilder) + db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None # skip the device-snapshot branch @@ -190,6 +200,7 @@ async def test_subscribe_events_includes_peers_snapshot_in_initial_state() -> No ``_pending_peers`` + ``_approved_peers`` dicts. """ db = DeviceBuilder.__new__(DeviceBuilder) + db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -247,6 +258,41 @@ async def test_subscribe_events_includes_peers_snapshot_in_initial_state() -> No await asyncio.gather(handler_task, return_exceptions=True) +async def test_subscribe_events_includes_preferences_in_initial_state(tmp_path) -> None: + """``_send_initial`` carries the UI-gating preferences in the snapshot. + + So first paint doesn't chase a separate ``get_preferences``. + """ + save_preferences( + tmp_path, + UserPreferences(remote_compute_only=True, experience_level=ExperienceLevel.YAML), + ) + db = DeviceBuilder.__new__(DeviceBuilder) + db.settings = MagicMock(config_dir=tmp_path) + db.bus = EventBus() + db.subscriber_presence = SubscriberPresence() + db.devices = None + db.remote_build_offloader = None + db.remote_build_receiver = None + + client = FakeWebSocketClient() + handler_task = asyncio.create_task(db._cmd_subscribe_events(client=client, message_id="m1")) + for _ in range(50): + await asyncio.sleep(0) + if client.events: + break + + initial_events = [e for e in client.events if e[1] == "initial_state"] + assert len(initial_events) == 1 + _, _, payload = initial_events[0] + prefs = payload["preferences"] + assert prefs["remote_compute_only"] is True + assert prefs["experience_level"] == "yaml" + + handler_task.cancel() + await asyncio.gather(handler_task, return_exceptions=True) + + async def test_subscribe_events_includes_offloader_settings_in_initial_state() -> None: """``_send_initial`` merges the offloader-wide settings into the seed. @@ -257,6 +303,7 @@ async def test_subscribe_events_includes_offloader_settings_in_initial_state() - (or in the picker's case never, since there's nothing to flip). """ db = DeviceBuilder.__new__(DeviceBuilder) + db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -301,6 +348,7 @@ async def test_subscribe_events_includes_hosts_snapshot_in_initial_state() -> No command. """ db = DeviceBuilder.__new__(DeviceBuilder) + db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -405,6 +453,7 @@ async def test_subscribe_events_subscribed_arrives_before_live_events() -> None: must be queued, then drained strictly after the seed. """ db = DeviceBuilder.__new__(DeviceBuilder) + db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() From fac604022359fe73ed50a1288c83957dd2b87a62 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 20:41:33 -0500 Subject: [PATCH 08/20] Trim the initial_state preferences comment Drop the parenthetical examples and the redundant tail; the docs carry the full rationale. --- esphome_device_builder/device_builder.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index d5ad8a0e3..51f83d929 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -596,10 +596,8 @@ async def _send_initial(_controls: StreamControls) -> None: # snapshot here a fresh page load would miss everything # the dashboard had already accumulated by then. initial: dict[str, Any] = {} - # Preferences gate first-paint UI (experience level, remote-compute - # creation hiding), so ship them in the snapshot rather than make the - # client chase a separate get_preferences round-trip. Sync read per - # the hot-path rule; the metadata sidecar is small. + # Gate first-paint UI, so ship them here instead of a separate + # get_preferences round-trip. Sync read per the hot-path rule. initial["preferences"] = load_preferences(self.settings.config_dir).to_dict() if self.devices: initial["devices"] = [d.to_dict() for d in self.devices.get_devices()] From acea02d04f37f2267659e78f5193a0c421a918ff Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 21:26:16 -0500 Subject: [PATCH 09/20] Make preferences RAM-canonical behind a PreferencesStore Reading prefs from disk inside _send_initial blocked the loop (blockbuster flags it) and a per-connect executor read would spawn an I/O-bound pool job on every WS connect. Move preferences to a dedicated debounced Store, the same pattern as the device-metadata and peer-link stores: load once at startup, migrate the _preferences blob out of the shared sidecar into .device-builder-preferences.json on first run, mutate RAM + debounce the write, and serve the subscribe snapshot sync from RAM. config/get_preferences and the onboarding reads/writes route through the store; the migration is covered like the metadata sidecar's. --- docs/API.md | 2 +- .../controllers/config/_preferences_store.py | 128 +++++++++++++++ .../controllers/config/controller.py | 25 ++- .../controllers/onboarding.py | 87 +++++----- esphome_device_builder/device_builder.py | 14 +- tests/controllers/config/__init__.py | 1 + .../config/test_preferences_store.py | 125 +++++++++++++++ tests/test_config_controller.py | 63 +++----- tests/test_onboarding_controller.py | 148 ++++++++++-------- tests/test_subscribe_events_cleanup.py | 36 +++-- 10 files changed, 452 insertions(+), 177 deletions(-) create mode 100644 esphome_device_builder/controllers/config/_preferences_store.py create mode 100644 tests/controllers/config/__init__.py create mode 100644 tests/controllers/config/test_preferences_store.py diff --git a/docs/API.md b/docs/API.md index c7036d594..93e681605 100644 --- a/docs/API.md +++ b/docs/API.md @@ -426,7 +426,7 @@ Same-subnet peers read `remote_build_port` from TXT so a `--remote-build-port` o **`subscribe_events` initial state:** -Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{preferences: UserPreferences, devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. `preferences` is always present — its `experience_level` and `remote_compute_only` fields gate first-paint UI (which editor surfaces show, whether device-creation entry points are hidden), so they ride the snapshot rather than make the client chase a separate `config/get_preferences`. The rest are present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync reads, no executor hop; the only disk touch is the small `preferences` sidecar (`load_preferences`). The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. +Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{preferences: UserPreferences, devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. `preferences` is always present — its `experience_level` and `remote_compute_only` fields gate first-paint UI (which editor surfaces show, whether device-creation entry points are hidden), so they ride the snapshot rather than make the client chase a separate `config/get_preferences`. The rest are present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync RAM reads, no executor hop, no disk I/O — `preferences` is RAM-canonical behind a `PreferencesStore` (loaded once at startup, mutations debounce a write to its own `.device-builder-preferences.json`, migrated out of the shared sidecar on first run; the same per-file `Store` pattern as the device-metadata and peer-link stores). The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. **`subscribe_events` events:** - `device_added`, `device_removed`, `device_updated`, `device_state_changed` diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py new file mode 100644 index 000000000..84a475e99 --- /dev/null +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -0,0 +1,128 @@ +"""RAM-canonical user preferences with a debounced disk write.""" + +from __future__ import annotations + +import asyncio +import logging +from collections.abc import Callable +from pathlib import Path +from typing import Any + +from ...helpers.json import JSONDecodeError, dumps_indent, loads +from ...helpers.storage import ShutdownRegister, Store +from ...models import UserPreferences +from .metadata import _load_metadata, metadata_transaction +from .preferences import _PREFS_KEY, _prefs_from_data + +_LOGGER = logging.getLogger(__name__) + +_STORE_FILENAME = ".device-builder-preferences.json" +_SHARED_SIDECAR_FILENAME = ".device-builder.json" + +_DEFAULT_SAVE_DELAY = 1.0 + + +def _encode(prefs: UserPreferences) -> bytes: + return dumps_indent(prefs.to_dict()) + + +def _decode(raw: bytes) -> UserPreferences: + try: + obj = loads(raw) + except JSONDecodeError: + _LOGGER.warning("preferences store: corrupt JSON, starting from defaults") + return UserPreferences() + if not isinstance(obj, dict): + return UserPreferences() + try: + return UserPreferences.from_dict(obj) + except (ValueError, TypeError, LookupError): + return UserPreferences() + + +class PreferencesStore: + """RAM-canonical user preferences; writes go through a debounced ``Store``.""" + + def __init__(self, config_dir: Path, shutdown_register: ShutdownRegister) -> None: + self._config_dir = config_dir + self._state = UserPreferences() + self._store: Store[UserPreferences] = Store( + config_dir / _STORE_FILENAME, + encoder=_encode, + decoder=_decode, + shutdown_register=shutdown_register, + name="preferences", + ) + + async def async_load(self) -> None: + """Seed RAM from disk; migrate the sidecar's ``_preferences`` on first run. + + Flushes the dedicated file before stripping the sidecar key so a crash + between the two preserves the migration. + """ + loaded = await self._store.async_load() + if loaded is not None: + self._state = loaded + return + loop = asyncio.get_running_loop() + migrated = await loop.run_in_executor(None, self._migrate_read_shared_sync) + if migrated is None: + return + self._state = migrated + self._store.async_delay_save(self._snapshot, delay=0.0) + await self._store.async_save_now() + await loop.run_in_executor(None, self._migrate_strip_shared_sync) + _LOGGER.info( + "Migrated preferences from %s to %s", _SHARED_SIDECAR_FILENAME, _STORE_FILENAME + ) + + def snapshot(self) -> UserPreferences: + """Return the current preferences from RAM (sync; for the subscribe snapshot).""" + return self._state + + def update( + self, fields: dict[str, Any], *, delay: float = _DEFAULT_SAVE_DELAY + ) -> UserPreferences: + """Merge a validated partial dict and schedule a debounced save.""" + new = UserPreferences.from_dict({**self._state.to_dict(), **fields}) + self._state = new + self._store.async_delay_save(self._snapshot, delay=delay) + return new + + def mutate( + self, + fn: Callable[[UserPreferences], UserPreferences | None], + *, + delay: float = _DEFAULT_SAVE_DELAY, + ) -> UserPreferences: + """Apply *fn* to a copy, replace RAM, schedule a save, return the result. + + *fn* may mutate the passed copy in place and return ``None`` (in-RAM + state is always replaced, never mutated in place, so a borrowed + :meth:`snapshot` reference stays stable). + """ + working = UserPreferences.from_dict(self._state.to_dict()) + result = fn(working) + if result is None: + result = working + self._state = result + self._store.async_delay_save(self._snapshot, delay=delay) + return result + + def _snapshot(self) -> UserPreferences: + return self._state + + def _migrate_read_shared_sync(self) -> UserPreferences | None: + """Decode the sidecar's ``_preferences`` blob, or ``None`` if absent.""" + shared_path = self._config_dir / _SHARED_SIDECAR_FILENAME + if not shared_path.exists(): + return None + data = _load_metadata(self._config_dir) + if _PREFS_KEY not in data: + return None + return _prefs_from_data(data) + + def _migrate_strip_shared_sync(self) -> None: + """Drop the migrated ``_preferences`` key from the shared sidecar.""" + with metadata_transaction(self._config_dir) as data: + data.pop(_PREFS_KEY, None) diff --git a/esphome_device_builder/controllers/config/controller.py b/esphome_device_builder/controllers/config/controller.py index 55ad55973..5ecad12c9 100644 --- a/esphome_device_builder/controllers/config/controller.py +++ b/esphome_device_builder/controllers/config/controller.py @@ -18,15 +18,16 @@ read_secrets_yaml, write_secret, ) +from ...helpers.storage import ShutdownCallback from ...helpers.storage_path import resolve_storage_path from ...models import ErrorCode, UserPreferences +from ._preferences_store import PreferencesStore from .chip_detect import ( _detect_chip_via_esptool, _detect_failure_message, _is_valid_port_name, _read_app_descriptor_board_id, ) -from .preferences import load_preferences, update_preferences if TYPE_CHECKING: from ...device_builder import DeviceBuilder @@ -37,6 +38,19 @@ class ConfigController: def __init__(self, device_builder: DeviceBuilder) -> None: self._db = device_builder + self._shutdown_callbacks: list[ShutdownCallback] = [] + self.prefs = PreferencesStore( + device_builder.settings.config_dir, self._shutdown_callbacks.append + ) + + async def async_load(self) -> None: + """Seed the RAM-canonical preferences store (and migrate on first run).""" + await self.prefs.async_load() + + async def stop(self) -> None: + """Flush the preferences store on shutdown.""" + for callback in self._shutdown_callbacks: + await callback() @api_command("config/version") async def get_version(self, **kwargs: Any) -> dict: @@ -94,9 +108,8 @@ async def detect_chip_cmd(self, **kwargs: Any) -> dict: @api_command("config/get_preferences") async def get_prefs(self, **kwargs: Any) -> UserPreferences: - """Get user preferences.""" - loop = asyncio.get_running_loop() - return await loop.run_in_executor(None, load_preferences, self._db.settings.config_dir) + """Get user preferences (RAM-canonical via the store).""" + return self.prefs.snapshot() @api_command("config/set_preferences") async def set_prefs(self, **kwargs: Any) -> UserPreferences: @@ -105,10 +118,8 @@ async def set_prefs(self, **kwargs: Any) -> UserPreferences: Accepts partial updates — only provided fields are changed, others keep their current values. """ - loop = asyncio.get_running_loop() - config_dir = self._db.settings.config_dir update_fields = {k: v for k, v in kwargs.items() if k not in ("client", "message_id")} - return await loop.run_in_executor(None, update_preferences, config_dir, update_fields) + return self.prefs.update(update_fields) @api_command("config/get_secrets") async def get_secrets(self, **kwargs: Any) -> list[str]: diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index 33fbd6662..998a840cc 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -21,7 +21,6 @@ import asyncio import logging -from functools import partial from pathlib import Path from typing import TYPE_CHECKING, Any @@ -43,12 +42,13 @@ UserPreferences, ) from ..models.onboarding import ONBOARDING_VERSION -from .config import load_preferences, mutate_preferences from .config.settings import _DASHBOARD_SENTINEL_FILE if TYPE_CHECKING: from esphome_device_builder.device_builder import DeviceBuilder + from .config._preferences_store import PreferencesStore + _LOGGER = logging.getLogger(__name__) @@ -66,6 +66,12 @@ class OnboardingController: def __init__(self, db: DeviceBuilder) -> None: self._db = db + @property + def _prefs(self) -> PreferencesStore: + # The config controller is created before onboarding in start(). + assert self._db.config is not None + return self._db.config.prefs + @api_command("onboarding/get_state") async def get_state(self, **kwargs: Any) -> OnboardingState: """ @@ -77,12 +83,11 @@ async def get_state(self, **kwargs: Any) -> OnboardingState: live data; the frontend surfaces the wizard on any pending step or a newer version. """ - loop = asyncio.get_running_loop() settings = self._db.settings - return await loop.run_in_executor( - None, - partial(_compute_state, settings.config_dir, on_ha_addon=settings.on_ha_addon), - ) + prefs = self._prefs.snapshot() + loop = asyncio.get_running_loop() + secrets = await loop.run_in_executor(None, read_secrets_yaml, settings.config_dir) + return _compute_state(secrets, prefs, on_ha_addon=settings.on_ha_addon) async def migrate_preexisting_install(self) -> None: """ @@ -93,8 +98,17 @@ async def migrate_preexisting_install(self) -> None: users and acknowledge onboarding so the wizard never auto-pops. Idempotent — a no-op once ``experience_level`` is set. """ - loop = asyncio.get_running_loop() - await loop.run_in_executor(None, _migrate_preexisting, self._db.settings.config_dir) + prefs = self._prefs.snapshot() + if prefs.experience_level is not None: + return + has_configs = False + if prefs.onboarding_completed_version == 0: + loop = asyncio.get_running_loop() + has_configs = await loop.run_in_executor( + None, _has_device_configs, self._db.settings.config_dir + ) + if _should_migrate_preexisting(prefs, has_device_configs=has_configs): + self._prefs.mutate(_mark_preexisting) @api_command("onboarding/set_wifi_credentials") async def set_wifi_credentials( @@ -171,23 +185,19 @@ async def mark_acknowledged(self, **kwargs: Any) -> OnboardingState: releases that add new steps bump that constant; existing users with a lower stored value will be re-prompted. """ - loop = asyncio.get_running_loop() - config_dir = self._db.settings.config_dir - await loop.run_in_executor( - None, mutate_preferences, config_dir, _acknowledge_current_version - ) + self._prefs.mutate(_acknowledge_current_version) return await self.get_state() -def _compute_state(config_dir: Path, *, on_ha_addon: bool) -> OnboardingState: +def _compute_state( + secrets: dict | None, prefs: UserPreferences, *, on_ha_addon: bool +) -> OnboardingState: """ - Build the onboarding snapshot in one executor hop. + Assemble the environment- and preference-aware onboarding step list. - Reads ``secrets.yaml`` + preferences (both quick reads from the - same dir) and assembles the environment-aware step list. + *secrets* and *prefs* are read by the caller (secrets off the loop, prefs + from the RAM-canonical store) so this stays pure. """ - secrets = read_secrets_yaml(config_dir) - prefs = load_preferences(config_dir) experience_done = _status(done=prefs.experience_level is not None) steps: list[OnboardingStep] = [] @@ -212,30 +222,25 @@ def _compute_state(config_dir: Path, *, on_ha_addon: bool) -> OnboardingState: ) -def _migrate_preexisting(config_dir: Path) -> None: - """ - Default a pre-existing install to the YAML experience; no-op otherwise. +def _should_migrate_preexisting(prefs: UserPreferences, *, has_device_configs: bool) -> bool: + """Whether a pre-existing install should default to the YAML experience. - Installs that already completed an earlier onboarding are also marked - acknowledged (their prior Wi-Fi choice stands); installs known only by a - device YAML are left un-acknowledged so a missing-Wi-Fi prompt still fires. + No-op once an experience is chosen; a fresh install with no device YAMLs and + no prior onboarding is left alone so the wizard still runs. """ - prefs = load_preferences(config_dir) if prefs.experience_level is not None: - return - if prefs.onboarding_completed_version == 0 and not _has_device_configs(config_dir): - return - - def _mark(p: UserPreferences) -> None: - p.experience_level = ExperienceLevel.YAML - # Only acknowledge onboarding for installs that already completed it, - # so a prior Wi-Fi save or decline is respected. An install known only - # by its device YAML stays un-acknowledged, so a missing-Wi-Fi prompt - # still surfaces. - if p.onboarding_completed_version > 0: - _acknowledge_current_version(p) - - mutate_preferences(config_dir, _mark) + return False + return prefs.onboarding_completed_version > 0 or has_device_configs + + +def _mark_preexisting(p: UserPreferences) -> None: + """Mark *p* a YAML user; acknowledge only if onboarding was already done.""" + p.experience_level = ExperienceLevel.YAML + # Only acknowledge onboarding for installs that already completed it, so a + # prior Wi-Fi save or decline is respected. An install known only by its + # device YAML stays un-acknowledged, so a missing-Wi-Fi prompt still surfaces. + if p.onboarding_completed_version > 0: + _acknowledge_current_version(p) def _acknowledge_current_version(prefs: UserPreferences) -> None: diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index 51f83d929..c2ce7f9bd 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -32,7 +32,6 @@ from .controllers.config import ( ConfigController, DashboardSettings, - load_preferences, ) from .controllers.devices import DevicesController from .controllers.editor import EditorController @@ -337,6 +336,9 @@ async def start(self) -> None: self.remote_build_offloader = OffloaderController(self) self.remote_build_receiver = ReceiverController(self) self.version_history = VersionHistoryController(self) + # Seed the RAM-canonical preferences (and migrate them out of the shared + # sidecar on first run) before onboarding reads or mutates them. + await self.config.async_load() # Default pre-existing installs to the YAML experience before # any onboarding command can be served. await self.onboarding.migrate_preexisting_install() @@ -436,7 +438,7 @@ async def start(self) -> None: len(self.command_handlers), ) - async def stop(self) -> None: # noqa: C901 + async def stop(self) -> None: # noqa: C901, PLR0912 """Shut down the application.""" _LOGGER.info("Shutting down ESPHome Device Builder") if self._bg_task: @@ -474,6 +476,8 @@ async def stop(self) -> None: # noqa: C901 await self.editor.stop() if self.version_history is not None: await self.version_history.stop() + if self.config is not None: + await self.config.stop() # Cleanly drain the pool once nothing else can hand it work. # Two paths because the pool is created eagerly in ``__init__`` # — calling ``stop()`` on an instance that never ran @@ -597,8 +601,10 @@ async def _send_initial(_controls: StreamControls) -> None: # the dashboard had already accumulated by then. initial: dict[str, Any] = {} # Gate first-paint UI, so ship them here instead of a separate - # get_preferences round-trip. Sync read per the hot-path rule. - initial["preferences"] = load_preferences(self.settings.config_dir).to_dict() + # get_preferences round-trip. Sync RAM read off the store; the + # config controller is always up by the time subscribe is served. + assert self.config is not None + initial["preferences"] = self.config.prefs.snapshot().to_dict() if self.devices: initial["devices"] = [d.to_dict() for d in self.devices.get_devices()] initial["importable"] = [d.to_dict() for d in self.devices.get_importable_devices()] diff --git a/tests/controllers/config/__init__.py b/tests/controllers/config/__init__.py new file mode 100644 index 000000000..6ed08b589 --- /dev/null +++ b/tests/controllers/config/__init__.py @@ -0,0 +1 @@ +"""Tests for the config controller.""" diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py new file mode 100644 index 000000000..80d4f19c4 --- /dev/null +++ b/tests/controllers/config/test_preferences_store.py @@ -0,0 +1,125 @@ +"""Tests for ``PreferencesStore`` — RAM-canonical prefs + sidecar migration.""" + +from __future__ import annotations + +import asyncio +from pathlib import Path + +import orjson +import pytest + +from esphome_device_builder.controllers.config import _load_metadata, _save_metadata +from esphome_device_builder.controllers.config._preferences_store import ( + _STORE_FILENAME, + PreferencesStore, +) +from esphome_device_builder.models import UserPreferences +from esphome_device_builder.models.preferences import ExperienceLevel, Theme + + +def _make_store(tmp_path: Path) -> PreferencesStore: + """Build a store anchored at *tmp_path* with a noop shutdown register.""" + return PreferencesStore(tmp_path, lambda _cb: None) + + +_SAMPLE = UserPreferences( + remote_compute_only=True, + experience_level=ExperienceLevel.YAML, + theme=Theme.DARK, +) + + +async def test_async_load_with_no_files_keeps_defaults(tmp_path: Path) -> None: + """Fresh install: no files, defaults in RAM, nothing written.""" + store = _make_store(tmp_path) + await store.async_load() + assert store.snapshot() == UserPreferences() + assert not (tmp_path / _STORE_FILENAME).exists() + assert not (tmp_path / ".device-builder.json").exists() + + +async def test_async_load_migrates_preferences_from_sidecar(tmp_path: Path) -> None: + """First run: ``_preferences`` moves to the dedicated file, other keys stay.""" + await asyncio.to_thread( + _save_metadata, + tmp_path, + { + "_preferences": _SAMPLE.to_dict(), + "_labels": [{"id": "abc", "name": "Bedroom"}], + "kitchen.yaml": {"board_id": "esp32"}, + }, + ) + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == _SAMPLE + assert (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert "_preferences" not in shared + assert shared["_labels"] == [{"id": "abc", "name": "Bedroom"}] + assert shared["kitchen.yaml"] == {"board_id": "esp32"} + + +async def test_async_load_skips_migration_when_dedicated_file_exists(tmp_path: Path) -> None: + """An existing dedicated file wins; the sidecar ``_preferences`` is left alone.""" + new = UserPreferences(theme=Theme.LIGHT) + (tmp_path / _STORE_FILENAME).write_bytes(orjson.dumps(new.to_dict())) + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == new + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == _SAMPLE.to_dict() + + +async def test_async_load_no_preferences_key_keeps_defaults(tmp_path: Path) -> None: + """A sidecar with other keys but no ``_preferences`` migrates nothing.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"kitchen.yaml": {"board_id": "esp32"}}) + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == UserPreferences() + assert not (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["kitchen.yaml"] == {"board_id": "esp32"} + + +async def test_async_load_is_idempotent(tmp_path: Path) -> None: + """A second load reads the dedicated file; the sidecar stays as migration left it.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + first = _make_store(tmp_path) + await first.async_load() + shared_after_first = await asyncio.to_thread(_load_metadata, tmp_path) + + second = _make_store(tmp_path) + await second.async_load() + assert second.snapshot() == first.snapshot() == _SAMPLE + assert await asyncio.to_thread(_load_metadata, tmp_path) == shared_after_first + + +async def test_async_load_recovers_from_corrupt_dedicated_file( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """A corrupt dedicated file falls back to defaults with a warning, no migration.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") + store = _make_store(tmp_path) + with caplog.at_level("WARNING"): + await store.async_load() + assert store.snapshot() == UserPreferences() + assert any("corrupt JSON" in r.message for r in caplog.records) + + +async def test_round_trip_after_migration(tmp_path: Path) -> None: + """Migrate, mutate, flush, reload: the latest state survives on disk.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + first = _make_store(tmp_path) + await first.async_load() + first.update({"theme": Theme.LIGHT}) + await first._store.async_save_now() + + second = _make_store(tmp_path) + await second.async_load() + assert second.snapshot().theme == Theme.LIGHT + assert second.snapshot().experience_level == ExperienceLevel.YAML diff --git a/tests/test_config_controller.py b/tests/test_config_controller.py index 2a2cba6a0..515aefbac 100644 --- a/tests/test_config_controller.py +++ b/tests/test_config_controller.py @@ -75,6 +75,7 @@ set_device_metadata, update_preferences, ) +from esphome_device_builder.controllers.config._preferences_store import PreferencesStore from esphome_device_builder.helpers.api import CommandError from esphome_device_builder.helpers.secrets_state import read_secrets_yaml from esphome_device_builder.models import ( @@ -95,8 +96,8 @@ from .conftest import MakeSettingsFactory, wire_secrets_writer -def _make_controller(config_dir: Path) -> ConfigController: - """Bypass __init__ chains; attach a stub DeviceBuilder.settings.""" +def _make_controller(config_dir: Path, *, prefs: UserPreferences | None = None) -> ConfigController: + """Bypass __init__ chains; attach a stub DeviceBuilder.settings + prefs store.""" controller = ConfigController.__new__(ConfigController) controller._db = MagicMock() controller._db.settings.config_dir = config_dir @@ -104,6 +105,11 @@ def _make_controller(config_dir: Path) -> ConfigController: controller._db.settings.rel_path = config_dir.joinpath controller._db.secrets_write_lock = asyncio.Lock() wire_secrets_writer(controller._db) + # RAM-canonical prefs store seeded in RAM (the debounce timer never fires + # within the test); get/set_prefs read and mutate the snapshot. + controller._shutdown_callbacks = [] + controller.prefs = PreferencesStore(config_dir, controller._shutdown_callbacks.append) + controller.prefs._state = prefs if prefs is not None else UserPreferences() return controller @@ -750,51 +756,25 @@ def test_save_preferences_round_trip(tmp_path: Path) -> None: async def test_get_prefs_returns_loaded_preferences(tmp_path: Path) -> None: - """``get_prefs`` returns the persisted blob, not a fresh default. - - Persists a non-default preferences object and asserts it - round-trips back. A regression that bypasses disk I/O and - just constructs ``UserPreferences()`` would still claim - ``isinstance`` but would fail this equality. - - The seeding ``save_preferences`` runs in a thread because - ``metadata_transaction`` -> ``tempfile.mkstemp`` -> - ``os.path.abspath`` blocks the event loop, and blockbuster - flags it from inside an async test. - """ + """``get_prefs`` returns the store's snapshot, not a fresh default.""" persisted = UserPreferences(dashboard_view=DashboardView.TABLE) - await asyncio.to_thread(save_preferences, tmp_path, persisted) - controller = _make_controller(tmp_path) + controller = _make_controller(tmp_path, prefs=persisted) prefs = await controller.get_prefs() assert prefs == persisted async def test_set_prefs_merges_partial_update(tmp_path: Path) -> None: - """Partial-update merge: only the supplied field changes. - - Persist a known initial state, then call ``set_prefs`` with - just one field. The unrelated fields must keep their - persisted values, not snap back to dataclass defaults — a - regression that re-constructs ``UserPreferences`` from - kwargs alone (skipping the merge step) would clobber them - silently. - - Seeding goes via ``asyncio.to_thread`` so the - ``metadata_transaction`` -> ``tempfile.mkstemp`` write - doesn't trip blockbuster on Linux CI. - """ + """Partial-update merge: only the supplied field changes; the rest survive.""" initial = UserPreferences(dashboard_view=DashboardView.TABLE, theme=Theme.DARK) - await asyncio.to_thread(save_preferences, tmp_path, initial) - controller = _make_controller(tmp_path) + controller = _make_controller(tmp_path, prefs=initial) # Update only ``theme``; ``dashboard_view`` should survive. result = await controller.set_prefs(theme=Theme.LIGHT) assert result.theme == Theme.LIGHT assert result.dashboard_view == DashboardView.TABLE - # Persisted blob matches the merged state. - persisted = await asyncio.to_thread(load_preferences, tmp_path) - assert persisted == result + # The snapshot reflects the merged state. + assert controller.prefs.snapshot() == result def test_update_preferences_defaults_on_corrupt(tmp_path: Path) -> None: @@ -809,11 +789,10 @@ def test_update_preferences_defaults_on_corrupt(tmp_path: Path) -> None: async def test_set_prefs_concurrent_updates_do_not_lose_writes(tmp_path: Path) -> None: - """Concurrent partial updates of distinct fields all survive. + """Partial updates of distinct fields all survive on the RAM-canonical store. - The read-merge-save runs inside one ``metadata_transaction``, - so two writers can't both read the same baseline and clobber - each other's field. A non-atomic load-then-save loses one. + Each ``set_prefs`` merges onto the current snapshot in RAM, so distinct + fields accumulate rather than clobbering each other. """ controller = _make_controller(tmp_path) @@ -823,10 +802,10 @@ async def test_set_prefs_concurrent_updates_do_not_lose_writes(tmp_path: Path) - controller.set_prefs(navigator_visible=False), ) - persisted = await asyncio.to_thread(load_preferences, tmp_path) - assert persisted.theme == Theme.DARK - assert persisted.dashboard_view == DashboardView.TABLE - assert persisted.navigator_visible is False + snap = controller.prefs.snapshot() + assert snap.theme == Theme.DARK + assert snap.dashboard_view == DashboardView.TABLE + assert snap.navigator_visible is False async def test_get_secrets_returns_empty_when_missing(tmp_path: Path) -> None: diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index 7ce4dcb6b..ef77266fa 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -15,14 +15,12 @@ import pytest -from esphome_device_builder.controllers.config import ( - load_preferences, - save_preferences, - update_preferences, -) +from esphome_device_builder.controllers.config._preferences_store import PreferencesStore from esphome_device_builder.controllers.onboarding import ( OnboardingController, _has_device_configs, + _mark_preexisting, + _should_migrate_preexisting, ) from esphome_device_builder.helpers.api import CommandError from esphome_device_builder.helpers.secrets_state import ( @@ -45,7 +43,9 @@ from .conftest import wire_secrets_writer -def _make_controller(config_dir: Path, *, on_ha_addon: bool = False) -> OnboardingController: +def _make_controller( + config_dir: Path, *, on_ha_addon: bool = False, prefs: UserPreferences | None = None +) -> OnboardingController: controller = OnboardingController.__new__(OnboardingController) controller._db = MagicMock() controller._db.settings.config_dir = config_dir @@ -53,6 +53,11 @@ def _make_controller(config_dir: Path, *, on_ha_addon: bool = False) -> Onboardi controller._db.settings.on_ha_addon = on_ha_addon controller._db.secrets_write_lock = asyncio.Lock() wire_secrets_writer(controller._db) + # RAM-canonical prefs store seeded in RAM; mutations stay in RAM (the + # debounce timer never fires within the test), asserted via the snapshot. + store = PreferencesStore(config_dir, lambda _cb: None) + store._state = prefs if prefs is not None else UserPreferences() + controller._db.config.prefs = store return controller @@ -136,8 +141,9 @@ async def test_get_state_experience_set_marks_use_case_and_experience_done( tmp_path: Path, ) -> None: """Picking an experience level completes both leading steps.""" - await asyncio.to_thread(update_preferences, tmp_path, {"experience_level": ExperienceLevel.UI}) - controller = _make_controller(tmp_path, on_ha_addon=False) + controller = _make_controller( + tmp_path, on_ha_addon=False, prefs=UserPreferences(experience_level=ExperienceLevel.UI) + ) state = await controller.get_state() assert _step(state, OnboardingStepId.USE_CASE) == OnboardingStepStatus.DONE assert _step(state, OnboardingStepId.EXPERIENCE_LEVEL) == OnboardingStepStatus.DONE @@ -145,8 +151,9 @@ async def test_get_state_experience_set_marks_use_case_and_experience_done( async def test_get_state_remote_compute_only_drops_wifi_step(tmp_path: Path) -> None: """A remote-compute-only install skips Wi-Fi setup entirely.""" - await asyncio.to_thread(update_preferences, tmp_path, {"remote_compute_only": True}) - controller = _make_controller(tmp_path, on_ha_addon=False) + controller = _make_controller( + tmp_path, on_ha_addon=False, prefs=UserPreferences(remote_compute_only=True) + ) state = await controller.get_state() assert _step(state, OnboardingStepId.WIFI_CREDENTIALS) is None assert OnboardingStepId.USE_CASE in [s.id for s in state.steps] @@ -281,9 +288,7 @@ async def test_mark_acknowledged_persists_current_version(tmp_path: Path) -> Non controller = _make_controller(tmp_path) state = await controller.mark_acknowledged() assert state.completed_version == ONBOARDING_VERSION - # Re-read on a fresh controller to confirm the prefs file landed. - state2 = await _make_controller(tmp_path).get_state() - assert state2.completed_version == ONBOARDING_VERSION + assert controller._prefs.snapshot().onboarding_completed_version == ONBOARDING_VERSION async def test_mark_acknowledged_is_idempotent(tmp_path: Path) -> None: @@ -293,25 +298,13 @@ async def test_mark_acknowledged_is_idempotent(tmp_path: Path) -> None: assert state.completed_version == ONBOARDING_VERSION -async def test_mark_acknowledged_does_not_clobber_a_concurrent_pref_write( - tmp_path: Path, -) -> None: - """Concurrent acknowledgement and a ``set_preferences`` write keep both fields. - - Both share the ``_preferences`` blob and go through - ``metadata_transaction``, so neither can read a stale baseline - and overwrite the other's field. - """ - controller = _make_controller(tmp_path) - - await asyncio.gather( - controller.mark_acknowledged(), - asyncio.to_thread(update_preferences, tmp_path, {"theme": Theme.DARK}), - ) - - persisted = await asyncio.to_thread(load_preferences, tmp_path) - assert persisted.onboarding_completed_version == ONBOARDING_VERSION - assert persisted.theme == Theme.DARK +async def test_mark_acknowledged_keeps_other_pref_fields(tmp_path: Path) -> None: + """Acknowledging touches only the version, leaving an unrelated field intact.""" + controller = _make_controller(tmp_path, prefs=UserPreferences(theme=Theme.DARK)) + await controller.mark_acknowledged() + snap = controller._prefs.snapshot() + assert snap.onboarding_completed_version == ONBOARDING_VERSION + assert snap.theme == Theme.DARK async def test_mark_acknowledged_does_not_downgrade_a_higher_stored_version( @@ -319,19 +312,14 @@ async def test_mark_acknowledged_does_not_downgrade_a_higher_stored_version( ) -> None: """Don't lose a future-build acknowledgement on rollback. - A user who briefly ran a future build with - ``ONBOARDING_VERSION = 2`` and then rolled back to this - build (``= 1``) keeps the higher stored value — otherwise - they'd be re-prompted on the next upgrade for steps they've - already done. + A user who briefly ran a future build with a higher + ``ONBOARDING_VERSION`` and then rolled back keeps the higher stored + value — otherwise they'd be re-prompted on the next upgrade for steps + they've already done. """ - future = UserPreferences(onboarding_completed_version=ONBOARDING_VERSION + 5) - # ``save_preferences`` does sync filesystem I/O that ``blockbuster`` - # rejects when called inline from an async test. Hop to an executor - # so we behave like the controller does in production. - loop = asyncio.get_running_loop() - await loop.run_in_executor(None, save_preferences, tmp_path, future) - controller = _make_controller(tmp_path) + controller = _make_controller( + tmp_path, prefs=UserPreferences(onboarding_completed_version=ONBOARDING_VERSION + 5) + ) state = await controller.mark_acknowledged() assert state.completed_version == ONBOARDING_VERSION + 5 @@ -567,11 +555,9 @@ async def test_migrate_acknowledged_install_becomes_yaml_and_stays_acknowledged( Their prior Wi-Fi save / decline stands, so onboarding is bumped to current. """ - await asyncio.to_thread( - save_preferences, tmp_path, UserPreferences(onboarding_completed_version=1) - ) - await _make_controller(tmp_path).migrate_preexisting_install() - prefs = await asyncio.to_thread(load_preferences, tmp_path) + controller = _make_controller(tmp_path, prefs=UserPreferences(onboarding_completed_version=1)) + await controller.migrate_preexisting_install() + prefs = controller._prefs.snapshot() assert prefs.experience_level == ExperienceLevel.YAML assert prefs.onboarding_completed_version == ONBOARDING_VERSION @@ -583,8 +569,9 @@ async def test_migrate_device_yaml_install_stays_unacknowledged(tmp_path: Path) still fire for these users. """ (tmp_path / "living-room.yaml").write_text("esphome:\n name: living-room\n") - await _make_controller(tmp_path).migrate_preexisting_install() - prefs = await asyncio.to_thread(load_preferences, tmp_path) + controller = _make_controller(tmp_path) + await controller.migrate_preexisting_install() + prefs = controller._prefs.snapshot() assert prefs.experience_level == ExperienceLevel.YAML assert prefs.onboarding_completed_version == 0 @@ -592,12 +579,42 @@ async def test_migrate_device_yaml_install_stays_unacknowledged(tmp_path: Path) async def test_migrate_install_with_yml_extension_becomes_yaml(tmp_path: Path) -> None: """``.yml`` is an equally valid config extension; it must trigger migration too.""" (tmp_path / "bedroom.yml").write_text("esphome:\n name: bedroom\n") - await _make_controller(tmp_path).migrate_preexisting_install() - prefs = await asyncio.to_thread(load_preferences, tmp_path) + controller = _make_controller(tmp_path) + await controller.migrate_preexisting_install() + prefs = controller._prefs.snapshot() assert prefs.experience_level == ExperienceLevel.YAML assert prefs.onboarding_completed_version == 0 +def test_should_migrate_preexisting_decision() -> None: + """The YAML-default decision: skip a chosen experience and a bare fresh install.""" + # Already chose an experience → never migrate. + assert not _should_migrate_preexisting( + UserPreferences(experience_level=ExperienceLevel.BEGINNER), has_device_configs=True + ) + # Unchosen + acknowledged earlier onboarding → migrate. + assert _should_migrate_preexisting( + UserPreferences(onboarding_completed_version=1), has_device_configs=False + ) + # Unchosen + has device YAMLs → migrate. + assert _should_migrate_preexisting(UserPreferences(), has_device_configs=True) + # Unchosen, never onboarded, no configs → fresh install, no migration. + assert not _should_migrate_preexisting(UserPreferences(), has_device_configs=False) + + +def test_mark_preexisting_acknowledges_only_a_completed_install() -> None: + """The marker sets YAML always, but acknowledges only a completed install.""" + completed = UserPreferences(onboarding_completed_version=1) + _mark_preexisting(completed) + assert completed.experience_level == ExperienceLevel.YAML + assert completed.onboarding_completed_version == ONBOARDING_VERSION + + config_only = UserPreferences() + _mark_preexisting(config_only) + assert config_only.experience_level == ExperienceLevel.YAML + assert config_only.onboarding_completed_version == 0 + + def test_has_device_configs_missing_dir_returns_false(tmp_path: Path) -> None: """A genuinely-absent config dir is a fresh install, not a scan failure.""" assert _has_device_configs(tmp_path / "does-not-exist") is False @@ -618,8 +635,9 @@ def test_has_device_configs_unreadable_dir_assumes_preexisting(tmp_path: Path) - async def test_migrate_fresh_install_is_noop(tmp_path: Path) -> None: """No prior onboarding and no device YAML ⇒ stay unchosen, see the wizard.""" - await _make_controller(tmp_path).migrate_preexisting_install() - prefs = await asyncio.to_thread(load_preferences, tmp_path) + controller = _make_controller(tmp_path) + await controller.migrate_preexisting_install() + prefs = controller._prefs.snapshot() assert prefs.experience_level is None assert prefs.onboarding_completed_version == 0 @@ -627,22 +645,22 @@ async def test_migrate_fresh_install_is_noop(tmp_path: Path) -> None: async def test_migrate_ignores_secrets_yaml(tmp_path: Path) -> None: """``secrets.yaml`` alone is not a device config — no migration.""" _write_secrets(tmp_path, "wifi_ssid: home\n") - await _make_controller(tmp_path).migrate_preexisting_install() - prefs = await asyncio.to_thread(load_preferences, tmp_path) - assert prefs.experience_level is None + controller = _make_controller(tmp_path) + await controller.migrate_preexisting_install() + assert controller._prefs.snapshot().experience_level is None async def test_migrate_preserves_an_explicit_choice(tmp_path: Path) -> None: """A user who already picked BEGINNER isn't overwritten by migration.""" - await asyncio.to_thread( - save_preferences, + (tmp_path / "device.yaml").write_text("esphome:\n name: device\n") + controller = _make_controller( tmp_path, - UserPreferences(experience_level=ExperienceLevel.BEGINNER, onboarding_completed_version=2), + prefs=UserPreferences( + experience_level=ExperienceLevel.BEGINNER, onboarding_completed_version=2 + ), ) - (tmp_path / "device.yaml").write_text("esphome:\n name: device\n") - await _make_controller(tmp_path).migrate_preexisting_install() - prefs = await asyncio.to_thread(load_preferences, tmp_path) - assert prefs.experience_level == ExperienceLevel.BEGINNER + await controller.migrate_preexisting_install() + assert controller._prefs.snapshot().experience_level == ExperienceLevel.BEGINNER # --------------------------------------------------------------------------- diff --git a/tests/test_subscribe_events_cleanup.py b/tests/test_subscribe_events_cleanup.py index 4486fdc91..19a75b77c 100644 --- a/tests/test_subscribe_events_cleanup.py +++ b/tests/test_subscribe_events_cleanup.py @@ -14,14 +14,11 @@ from __future__ import annotations import asyncio -import tempfile -from pathlib import Path from typing import Any from unittest.mock import MagicMock import pytest -from esphome_device_builder.controllers.config import save_preferences from esphome_device_builder.device_builder import DeviceBuilder from esphome_device_builder.helpers.event_bus import ( _DEFAULT_STREAM_QUEUE_MAX, @@ -34,9 +31,15 @@ from .conftest import FakeWebSocketClient -# ``_send_initial`` reads preferences off ``settings.config_dir``; an empty dir -# makes ``load_preferences`` return defaults, which is all these stubs need. -_STUB_CONFIG_DIR = Path(tempfile.mkdtemp()) + +def _stub_config(db: DeviceBuilder, prefs: UserPreferences | None = None) -> None: + """Give *db* a config controller whose prefs store returns *prefs*. + + ``_send_initial`` reads the snapshot off ``config.prefs``; default it to + plain defaults, which is all most of these stubs need. + """ + db.config = MagicMock() + db.config.prefs.snapshot.return_value = prefs or UserPreferences() def _make_db() -> DeviceBuilder: @@ -48,7 +51,7 @@ def _make_db() -> DeviceBuilder: stub. """ db = DeviceBuilder.__new__(DeviceBuilder) - db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None # skip the device-snapshot branch @@ -152,7 +155,7 @@ async def test_subscribe_events_includes_pairings_snapshot_in_initial_state() -> hop, no disk read. """ db = DeviceBuilder.__new__(DeviceBuilder) - db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None # skip the device-snapshot branch @@ -200,7 +203,7 @@ async def test_subscribe_events_includes_peers_snapshot_in_initial_state() -> No ``_pending_peers`` + ``_approved_peers`` dicts. """ db = DeviceBuilder.__new__(DeviceBuilder) - db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -258,17 +261,16 @@ async def test_subscribe_events_includes_peers_snapshot_in_initial_state() -> No await asyncio.gather(handler_task, return_exceptions=True) -async def test_subscribe_events_includes_preferences_in_initial_state(tmp_path) -> None: +async def test_subscribe_events_includes_preferences_in_initial_state() -> None: """``_send_initial`` carries the UI-gating preferences in the snapshot. So first paint doesn't chase a separate ``get_preferences``. """ - save_preferences( - tmp_path, + db = DeviceBuilder.__new__(DeviceBuilder) + _stub_config( + db, UserPreferences(remote_compute_only=True, experience_level=ExperienceLevel.YAML), ) - db = DeviceBuilder.__new__(DeviceBuilder) - db.settings = MagicMock(config_dir=tmp_path) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -303,7 +305,7 @@ async def test_subscribe_events_includes_offloader_settings_in_initial_state() - (or in the picker's case never, since there's nothing to flip). """ db = DeviceBuilder.__new__(DeviceBuilder) - db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -348,7 +350,7 @@ async def test_subscribe_events_includes_hosts_snapshot_in_initial_state() -> No command. """ db = DeviceBuilder.__new__(DeviceBuilder) - db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() db.devices = None @@ -453,7 +455,7 @@ async def test_subscribe_events_subscribed_arrives_before_live_events() -> None: must be queued, then drained strictly after the seed. """ db = DeviceBuilder.__new__(DeviceBuilder) - db.settings = MagicMock(config_dir=_STUB_CONFIG_DIR) + _stub_config(db) db.bus = EventBus() db.subscriber_presence = SubscriberPresence() From 1ecc91c502e66ed7761c23d6d23c56ee25290050 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 21:52:53 -0500 Subject: [PATCH 10/20] Delete orphaned sidecar prefs writers and harden the store decoder With preferences RAM-canonical behind the store, the sidecar writers (save/update/mutate/load_preferences) have no production callers; remove them so nothing can reintroduce a divergent _preferences blob into the sidecar (keep _PREFS_KEY + _prefs_from_data, which the migration still reads). snapshot() returns a copy so a caller can't mutate the canonical RAM state and skip the debounced write, and the decoder now logs on a non-object or undecodable payload instead of silently resetting to defaults. Adds the missing-coverage tests (decoder branches, ConfigController load/flush lifecycle, corrupt-sidecar migration). --- .../controllers/config/__init__.py | 12 +--- .../controllers/config/_preferences_store.py | 10 ++- .../controllers/config/preferences.py | 49 ++------------ .../config/test_preferences_store.py | 28 ++++++++ tests/test_config_controller.py | 67 ++++--------------- 5 files changed, 57 insertions(+), 109 deletions(-) diff --git a/esphome_device_builder/controllers/config/__init__.py b/esphome_device_builder/controllers/config/__init__.py index e61457dfa..8e11d2fa1 100644 --- a/esphome_device_builder/controllers/config/__init__.py +++ b/esphome_device_builder/controllers/config/__init__.py @@ -56,13 +56,7 @@ rename_device_metadata, set_device_metadata, ) -from .preferences import ( - _prefs_from_data, - load_preferences, - mutate_preferences, - save_preferences, - update_preferences, -) +from .preferences import _prefs_from_data from .remote_build_settings import ( _settings_from_raw, has_remote_build_settings_persisted, @@ -114,17 +108,13 @@ "has_remote_build_settings_persisted", "labels_transaction", "load_labels", - "load_preferences", "load_remote_build_settings", "metadata_transaction", - "mutate_preferences", "remote_build_settings_transaction", "remove_device_metadata", "rename_device_metadata", "save_labels", - "save_preferences", "save_remote_build_settings", "set_device_labels", "set_device_metadata", - "update_preferences", ] diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index 84a475e99..605f42946 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -33,10 +33,12 @@ def _decode(raw: bytes) -> UserPreferences: _LOGGER.warning("preferences store: corrupt JSON, starting from defaults") return UserPreferences() if not isinstance(obj, dict): + _LOGGER.warning("preferences store: non-object payload, starting from defaults") return UserPreferences() try: return UserPreferences.from_dict(obj) except (ValueError, TypeError, LookupError): + _LOGGER.exception("preferences store: undecodable payload, starting from defaults") return UserPreferences() @@ -77,8 +79,12 @@ async def async_load(self) -> None: ) def snapshot(self) -> UserPreferences: - """Return the current preferences from RAM (sync; for the subscribe snapshot).""" - return self._state + """Return a copy of the current preferences (sync; for the subscribe snapshot). + + A copy so a caller mutating it can't corrupt the canonical RAM state + (which would skip the debounced write and be lost on restart). + """ + return UserPreferences.from_dict(self._state.to_dict()) def update( self, fields: dict[str, Any], *, delay: float = _DEFAULT_SAVE_DELAY diff --git a/esphome_device_builder/controllers/config/preferences.py b/esphome_device_builder/controllers/config/preferences.py index 6e6a6e763..7965fd3a0 100644 --- a/esphome_device_builder/controllers/config/preferences.py +++ b/esphome_device_builder/controllers/config/preferences.py @@ -1,13 +1,16 @@ -"""User-preferences persistence backed by the metadata sidecar.""" +"""Decode the legacy ``_preferences`` sidecar blob (the migration source). + +Preferences are RAM-canonical behind ``PreferencesStore`` now; these are the +only bits that survive: the sidecar key + decoder the store reads once at +migration time. The sidecar *writers* were removed so nothing can reintroduce a +``_preferences`` blob that would diverge from the store. +""" from __future__ import annotations -from collections.abc import Callable -from pathlib import Path from typing import Any from ...models import UserPreferences -from .metadata import _load_metadata, metadata_transaction _PREFS_KEY = "_preferences" @@ -18,41 +21,3 @@ def _prefs_from_data(data: dict[str, Any]) -> UserPreferences: return UserPreferences.from_dict(data.get(_PREFS_KEY, {})) except (ValueError, TypeError, LookupError): return UserPreferences() - - -def load_preferences(config_dir: Path) -> UserPreferences: - """Load user preferences, returning defaults for missing or corrupt fields.""" - return _prefs_from_data(_load_metadata(config_dir)) - - -def save_preferences(config_dir: Path, prefs: UserPreferences) -> None: - """Save user preferences to disk.""" - with metadata_transaction(config_dir) as data: - data[_PREFS_KEY] = prefs.to_dict() - - -def mutate_preferences( - config_dir: Path, mutate: Callable[[UserPreferences], UserPreferences | None] -) -> UserPreferences: - """ - Atomic read-modify-write for user preferences. - - *mutate* receives the current prefs (defaults on a corrupt - blob) and returns the next state, or mutates it in place and - returns ``None``. Load and save share one lock. - """ - with metadata_transaction(config_dir) as data: - prefs = _prefs_from_data(data) - result = mutate(prefs) - if result is None: - result = prefs - data[_PREFS_KEY] = result.to_dict() - return result - - -def update_preferences(config_dir: Path, update_fields: dict[str, Any]) -> UserPreferences: - """Merge a validated partial dict into stored preferences atomically.""" - return mutate_preferences( - config_dir, - lambda prefs: UserPreferences.from_dict({**prefs.to_dict(), **update_fields}), - ) diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index 80d4f19c4..ae432c806 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -60,6 +60,18 @@ async def test_async_load_migrates_preferences_from_sidecar(tmp_path: Path) -> N assert shared["kitchen.yaml"] == {"board_id": "esp32"} +async def test_async_load_migrates_corrupt_sidecar_blob_as_defaults(tmp_path: Path) -> None: + """A malformed sidecar ``_preferences`` blob migrates to defaults, not a crash.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": [1, 2, 3]}) + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == UserPreferences() + assert (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert "_preferences" not in shared + + async def test_async_load_skips_migration_when_dedicated_file_exists(tmp_path: Path) -> None: """An existing dedicated file wins; the sidecar ``_preferences`` is left alone.""" new = UserPreferences(theme=Theme.LIGHT) @@ -111,6 +123,22 @@ async def test_async_load_recovers_from_corrupt_dedicated_file( assert any("corrupt JSON" in r.message for r in caplog.records) +async def test_async_load_recovers_from_non_dict_dedicated_file(tmp_path: Path) -> None: + """A dedicated file holding a non-object JSON value falls back to defaults.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"[1, 2, 3]") + store = _make_store(tmp_path) + await store.async_load() + assert store.snapshot() == UserPreferences() + + +async def test_async_load_recovers_from_invalid_prefs_shape(tmp_path: Path) -> None: + """A dedicated file whose object fails decode falls back to defaults.""" + (tmp_path / _STORE_FILENAME).write_bytes(b'{"experience_level": "bogus"}') + store = _make_store(tmp_path) + await store.async_load() + assert store.snapshot() == UserPreferences() + + async def test_round_trip_after_migration(tmp_path: Path) -> None: """Migrate, mutate, flush, reload: the latest state survives on disk.""" await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) diff --git a/tests/test_config_controller.py b/tests/test_config_controller.py index 515aefbac..5ac219cef 100644 --- a/tests/test_config_controller.py +++ b/tests/test_config_controller.py @@ -63,17 +63,14 @@ has_remote_build_settings_persisted, labels_transaction, load_labels, - load_preferences, load_remote_build_settings, metadata_transaction, remote_build_settings_transaction, remove_device_metadata, save_labels, - save_preferences, save_remote_build_settings, set_device_labels, set_device_metadata, - update_preferences, ) from esphome_device_builder.controllers.config._preferences_store import PreferencesStore from esphome_device_builder.helpers.api import CommandError @@ -464,33 +461,6 @@ def test_remove_device_metadata_clears_only_target(tmp_path: Path) -> None: assert get_device_metadata(tmp_path, "b.yaml") == {"board_id": "esp8266"} -def test_load_preferences_returns_defaults_on_missing(tmp_path: Path) -> None: - """A fresh install has no preferences — fall back to the default object. - - Equality check (not just ``isinstance``) so a regression - that builds a non-default preferences object on the - missing-file path (e.g. one with ``dashboard_view=TABLE`` - instead of CARDS) breaks this test. - """ - assert load_preferences(tmp_path) == UserPreferences() - - -def test_load_preferences_returns_defaults_on_bad_data(tmp_path: Path) -> None: - """Corrupted preferences blob → default object, not partial recovery. - - ``UserPreferences.from_dict`` raises on unknown / malformed - fields; without the except-fallback the dashboard wouldn't - load when an older version's preferences file is read by a - newer mashumaro schema. Equality with ``UserPreferences()`` - pins that the fallback is the same default object, not a - silently-mutated one a regression could produce. - """ - metadata_path = tmp_path / ".device-builder.json" - metadata_path.write_bytes(b'{"_preferences": {"unknown_field": 42}}') - - assert load_preferences(tmp_path) == UserPreferences() - - def test_load_remote_build_settings_returns_defaults_on_missing(tmp_path: Path) -> None: """Fresh config dir → ``RemoteBuildSettings()`` with ``enabled=True``. @@ -737,24 +707,24 @@ def test_remote_build_settings_transaction_discards_on_exception( assert load_remote_build_settings(tmp_path) == RemoteBuildSettings(enabled=False) -def test_save_preferences_round_trip(tmp_path: Path) -> None: - """A non-default prefs blob round-trips through save → load. - - Pins the actual write path: round-tripping ``UserPreferences()`` - would also pass if save / load both silently lost data, so - use a non-default value (``dashboard_view=TABLE``) to - actually exercise the marshalling. - """ - prefs = UserPreferences(dashboard_view=DashboardView.TABLE) - save_preferences(tmp_path, prefs) - assert load_preferences(tmp_path) == prefs - - # --------------------------------------------------------------------------- # ConfigController WS commands — verifies file I/O runs off the event loop # --------------------------------------------------------------------------- +async def test_config_controller_loads_and_flushes_preferences(tmp_path: Path) -> None: + """A real ``ConfigController`` builds the store, loads it, and flushes on stop.""" + db = MagicMock() + db.settings.config_dir = tmp_path + controller = ConfigController(db) # real __init__ builds the store + await controller.async_load() # no sidecar → defaults + assert controller.prefs.snapshot() == UserPreferences() + # A write schedules a debounced save; stop() flushes it to disk. + controller.prefs.update({"theme": Theme.DARK}) + await controller.stop() + assert (tmp_path / ".device-builder-preferences.json").exists() + + async def test_get_prefs_returns_loaded_preferences(tmp_path: Path) -> None: """``get_prefs`` returns the store's snapshot, not a fresh default.""" persisted = UserPreferences(dashboard_view=DashboardView.TABLE) @@ -777,17 +747,6 @@ async def test_set_prefs_merges_partial_update(tmp_path: Path) -> None: assert controller.prefs.snapshot() == result -def test_update_preferences_defaults_on_corrupt(tmp_path: Path) -> None: - """A malformed ``_preferences`` blob merges onto defaults, then persists clean.""" - metadata_path = tmp_path / ".device-builder.json" - metadata_path.write_bytes(b'{"_preferences": [1, 2, 3]}') - - updated = update_preferences(tmp_path, {"theme": Theme.DARK}) - - assert updated == UserPreferences(theme=Theme.DARK) - assert load_preferences(tmp_path) == UserPreferences(theme=Theme.DARK) - - async def test_set_prefs_concurrent_updates_do_not_lose_writes(tmp_path: Path) -> None: """Partial updates of distinct fields all survive on the RAM-canonical store. From 6ca20cad1bf3e73a7ff079d5bdbdcecce8381130 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 21:58:00 -0500 Subject: [PATCH 11/20] Extract the prefs-copy idiom and trim the migration-helper docstring snapshot() and mutate() both copied the canonical state via from_dict(to_dict(...)); hoist it into _copy(). Drop the deletion-history prose from preferences.py's docstring. --- .../controllers/config/_preferences_store.py | 8 ++++++-- esphome_device_builder/controllers/config/preferences.py | 8 +------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index 605f42946..bb01dfce8 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -84,7 +84,7 @@ def snapshot(self) -> UserPreferences: A copy so a caller mutating it can't corrupt the canonical RAM state (which would skip the debounced write and be lost on restart). """ - return UserPreferences.from_dict(self._state.to_dict()) + return self._copy() def update( self, fields: dict[str, Any], *, delay: float = _DEFAULT_SAVE_DELAY @@ -107,7 +107,7 @@ def mutate( state is always replaced, never mutated in place, so a borrowed :meth:`snapshot` reference stays stable). """ - working = UserPreferences.from_dict(self._state.to_dict()) + working = self._copy() result = fn(working) if result is None: result = working @@ -115,6 +115,10 @@ def mutate( self._store.async_delay_save(self._snapshot, delay=delay) return result + def _copy(self) -> UserPreferences: + """Return a fresh, independent copy of the canonical RAM state.""" + return UserPreferences.from_dict(self._state.to_dict()) + def _snapshot(self) -> UserPreferences: return self._state diff --git a/esphome_device_builder/controllers/config/preferences.py b/esphome_device_builder/controllers/config/preferences.py index 7965fd3a0..66c6d37d1 100644 --- a/esphome_device_builder/controllers/config/preferences.py +++ b/esphome_device_builder/controllers/config/preferences.py @@ -1,10 +1,4 @@ -"""Decode the legacy ``_preferences`` sidecar blob (the migration source). - -Preferences are RAM-canonical behind ``PreferencesStore`` now; these are the -only bits that survive: the sidecar key + decoder the store reads once at -migration time. The sidecar *writers* were removed so nothing can reintroduce a -``_preferences`` blob that would diverge from the store. -""" +"""Decode the legacy sidecar ``_preferences`` blob (read once at migration).""" from __future__ import annotations From 7d50de9e1d65edbceec08ed0f7abe84ca05a73f1 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 22:16:39 -0500 Subject: [PATCH 12/20] Preserve undecodable preferences instead of silently destroying them A corrupt or schema-incompatible prefs blob was defaulted silently and the source then erased: the migration stripped the sidecar _preferences key and the next save overwrote a corrupt dedicated file. Make _decode strict (propagate), preserve a corrupt dedicated file by renaming it .corrupt, and leave an undecodable legacy sidecar blob in place (skip the strip); both log before falling back to defaults. Drops the now-unused _prefs_from_data. --- docs/API.md | 2 +- .../controllers/config/__init__.py | 2 - .../controllers/config/_preferences_store.py | 68 +++++++++++++------ .../controllers/config/preferences.py | 14 +--- .../config/test_preferences_store.py | 37 ++++++---- 5 files changed, 75 insertions(+), 48 deletions(-) diff --git a/docs/API.md b/docs/API.md index 93e681605..d6cf155a8 100644 --- a/docs/API.md +++ b/docs/API.md @@ -426,7 +426,7 @@ Same-subnet peers read `remote_build_port` from TXT so a `--remote-build-port` o **`subscribe_events` initial state:** -Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{preferences: UserPreferences, devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. `preferences` is always present — its `experience_level` and `remote_compute_only` fields gate first-paint UI (which editor surfaces show, whether device-creation entry points are hidden), so they ride the snapshot rather than make the client chase a separate `config/get_preferences`. The rest are present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync RAM reads, no executor hop, no disk I/O — `preferences` is RAM-canonical behind a `PreferencesStore` (loaded once at startup, mutations debounce a write to its own `.device-builder-preferences.json`, migrated out of the shared sidecar on first run; the same per-file `Store` pattern as the device-metadata and peer-link stores). The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. +Right after a client subscribes (and before any live events arrive), the server pushes one `initial_state` event carrying a snapshot of state that's accumulated server-side via background activity (mDNS browser, completed pair flows, etc.) so the frontend can paint the first frame without follow-up reads. Shape: `{preferences: UserPreferences, devices?: [...], importable?: [...], pairings?: [PairingSummary], peers?: [PeerSummary], hosts?: [RemoteBuildPeer], offloader_alerts?: [OffloaderAlertSnapshotEntry], peer_queue_status?: [PeerQueueStatusSnapshotEntry], remote_jobs?: [OffloaderRemoteJobSnapshotEntry], remote_builds_enabled?: bool, version_match_policy?: "any" | "release" | "exact" | "exact_required"}`. `preferences` is always present — its `experience_level` and `remote_compute_only` fields gate first-paint UI (which editor surfaces show, whether device-creation entry points are hidden), so they ride the snapshot rather than make the client chase a separate `config/get_preferences`. The rest are present only when the corresponding controller is up; `pairings` carries both PENDING and APPROVED offloader-side rows from the `_pairings` dict, `peers` carries both PENDING (`_pending_peers`) and APPROVED (`_approved_peers`) receiver-side rows, `hosts` carries the receiver controller's mDNS-discovered peer dashboards (`self._peers`, RAM-only — never persisted), `offloader_alerts` carries the offloader-side pair alerts dict (`_offloader_alerts`, RAM-only) so a tab subscribing AFTER a `pin_mismatch` / `peer_revoked` event fired still renders the alert it would have missed on the live stream — the alert only clears via re-pair or unpair, never by an operator-driven dismiss, because the underlying state (broken pairing) doesn't fix itself. `peer_queue_status` carries the most recent `queue_status` snapshot per paired receiver so a late tab paints the per-peer queue depth without waiting for the next event. `remote_jobs` carries every offloader-submitted job that's still in flight (terminal entries drop on the matching `job_state_changed` event) so the UI can render running builds on page load. `remote_builds_enabled` and `version_match_policy` carry the current value of the offloader-wide settings so the Settings dialog renders both controls on first paint instead of waiting for the matching `offloader_remote_builds_toggled` / `offloader_version_match_policy_changed` event to fire. All sync RAM reads, no executor hop, no disk I/O — `preferences` is RAM-canonical behind a `PreferencesStore` (loaded once at startup, mutations debounce a write to its own `.device-builder-preferences.json`, migrated out of the shared sidecar on first run; the same per-file `Store` pattern as the device-metadata and peer-link stores). Undecodable preferences are preserved, never destroyed: a corrupt dedicated file is renamed to `.corrupt` and an undecodable legacy sidecar blob is left in place (not stripped), both logged, before the store falls back to defaults. The `PeerSummary` projection persists `peer_ip` (the source IP observed at pair_request time) on `StoredPeer` so a snapshot-loaded inbox row carries the same IP the live `remote_build_pair_request_received` event would carry; that's what the receiver Settings UI renders alongside the pin as a clone-risk sanity-check. Empty string for legacy on-disk rows from receivers that pre-date the field. Live updates that arrive after the initial state mutate against this seed via the events below. **`subscribe_events` events:** - `device_added`, `device_removed`, `device_updated`, `device_state_changed` diff --git a/esphome_device_builder/controllers/config/__init__.py b/esphome_device_builder/controllers/config/__init__.py index 8e11d2fa1..444dece3c 100644 --- a/esphome_device_builder/controllers/config/__init__.py +++ b/esphome_device_builder/controllers/config/__init__.py @@ -56,7 +56,6 @@ rename_device_metadata, set_device_metadata, ) -from .preferences import _prefs_from_data from .remote_build_settings import ( _settings_from_raw, has_remote_build_settings_persisted, @@ -93,7 +92,6 @@ "_make_descriptor_tempfile", "_parse_chip_family_line", "_parse_project_name", - "_prefs_from_data", "_read_app_descriptor_board_id", "_read_descriptor_file", "_run_esptool", diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index bb01dfce8..02a0de445 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -5,6 +5,7 @@ import asyncio import logging from collections.abc import Callable +from contextlib import suppress from pathlib import Path from typing import Any @@ -12,7 +13,7 @@ from ...helpers.storage import ShutdownRegister, Store from ...models import UserPreferences from .metadata import _load_metadata, metadata_transaction -from .preferences import _PREFS_KEY, _prefs_from_data +from .preferences import _PREFS_KEY _LOGGER = logging.getLogger(__name__) @@ -21,25 +22,25 @@ _DEFAULT_SAVE_DELAY = 1.0 +# Decode failures that should be treated as "corrupt / incompatible": a bad-JSON +# read, a non-object payload, or a shape ``from_dict`` rejects. +_DECODE_ERRORS = (JSONDecodeError, ValueError, TypeError, LookupError) + def _encode(prefs: UserPreferences) -> bytes: return dumps_indent(prefs.to_dict()) def _decode(raw: bytes) -> UserPreferences: - try: - obj = loads(raw) - except JSONDecodeError: - _LOGGER.warning("preferences store: corrupt JSON, starting from defaults") - return UserPreferences() + """Decode stored preferences; raise on a corrupt or incompatible payload. + + Corruption propagates (rather than defaulting) so the caller can preserve + the file for recovery instead of silently overwriting it. + """ + obj = loads(raw) if not isinstance(obj, dict): - _LOGGER.warning("preferences store: non-object payload, starting from defaults") - return UserPreferences() - try: - return UserPreferences.from_dict(obj) - except (ValueError, TypeError, LookupError): - _LOGGER.exception("preferences store: undecodable payload, starting from defaults") - return UserPreferences() + raise TypeError("preferences payload is not a JSON object") + return UserPreferences.from_dict(obj) class PreferencesStore: @@ -59,14 +60,25 @@ def __init__(self, config_dir: Path, shutdown_register: ShutdownRegister) -> Non async def async_load(self) -> None: """Seed RAM from disk; migrate the sidecar's ``_preferences`` on first run. - Flushes the dedicated file before stripping the sidecar key so a crash - between the two preserves the migration. + Undecodable data is preserved, never destroyed: a corrupt dedicated file + is renamed aside, and an undecodable legacy sidecar blob is left in place + (not stripped). Both log and fall back to defaults. Migration flushes the + dedicated file before stripping the sidecar key so a crash between the + two preserves the migration. """ - loaded = await self._store.async_load() + loop = asyncio.get_running_loop() + try: + loaded = await self._store.async_load() + except _DECODE_ERRORS: + _LOGGER.exception( + "preferences store: %s is undecodable; preserving it and using defaults", + _STORE_FILENAME, + ) + await loop.run_in_executor(None, self._preserve_corrupt_file) + return if loaded is not None: self._state = loaded return - loop = asyncio.get_running_loop() migrated = await loop.run_in_executor(None, self._migrate_read_shared_sync) if migrated is None: return @@ -122,15 +134,33 @@ def _copy(self) -> UserPreferences: def _snapshot(self) -> UserPreferences: return self._state + def _preserve_corrupt_file(self) -> None: + """Rename the undecodable dedicated file aside so the next save can't erase it.""" + path = self._config_dir / _STORE_FILENAME + with suppress(OSError): + path.replace(path.with_name(path.name + ".corrupt")) + def _migrate_read_shared_sync(self) -> UserPreferences | None: - """Decode the sidecar's ``_preferences`` blob, or ``None`` if absent.""" + """Decode the sidecar's ``_preferences`` blob. + + Returns ``None`` when the key is absent or undecodable; an undecodable + legacy blob is logged and left in the sidecar (the caller doesn't strip + it) so the data stays recoverable rather than being replaced by defaults. + """ shared_path = self._config_dir / _SHARED_SIDECAR_FILENAME if not shared_path.exists(): return None data = _load_metadata(self._config_dir) if _PREFS_KEY not in data: return None - return _prefs_from_data(data) + try: + return UserPreferences.from_dict(data[_PREFS_KEY]) + except _DECODE_ERRORS: + _LOGGER.exception( + "preferences store: legacy _preferences blob undecodable; left in %s for recovery", + _SHARED_SIDECAR_FILENAME, + ) + return None def _migrate_strip_shared_sync(self) -> None: """Drop the migrated ``_preferences`` key from the shared sidecar.""" diff --git a/esphome_device_builder/controllers/config/preferences.py b/esphome_device_builder/controllers/config/preferences.py index 66c6d37d1..862c25cae 100644 --- a/esphome_device_builder/controllers/config/preferences.py +++ b/esphome_device_builder/controllers/config/preferences.py @@ -1,17 +1,5 @@ -"""Decode the legacy sidecar ``_preferences`` blob (read once at migration).""" +"""Legacy sidecar key the preferences migration reads from.""" from __future__ import annotations -from typing import Any - -from ...models import UserPreferences - _PREFS_KEY = "_preferences" - - -def _prefs_from_data(data: dict[str, Any]) -> UserPreferences: - """Decode the ``_preferences`` blob, returning defaults on a corrupt shape.""" - try: - return UserPreferences.from_dict(data.get(_PREFS_KEY, {})) - except (ValueError, TypeError, LookupError): - return UserPreferences() diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index ae432c806..40e91b73e 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -60,16 +60,20 @@ async def test_async_load_migrates_preferences_from_sidecar(tmp_path: Path) -> N assert shared["kitchen.yaml"] == {"board_id": "esp32"} -async def test_async_load_migrates_corrupt_sidecar_blob_as_defaults(tmp_path: Path) -> None: - """A malformed sidecar ``_preferences`` blob migrates to defaults, not a crash.""" +async def test_async_load_leaves_corrupt_sidecar_blob_in_place(tmp_path: Path) -> None: + """A malformed sidecar ``_preferences`` blob is preserved, not migrated/stripped. + + Falling back to defaults *and* destroying the source would lose recoverable + data; instead leave the blob in the sidecar and don't write a dedicated file. + """ await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": [1, 2, 3]}) store = _make_store(tmp_path) await store.async_load() assert store.snapshot() == UserPreferences() - assert (tmp_path / _STORE_FILENAME).exists() + assert not (tmp_path / _STORE_FILENAME).exists() shared = await asyncio.to_thread(_load_metadata, tmp_path) - assert "_preferences" not in shared + assert shared["_preferences"] == [1, 2, 3] async def test_async_load_skips_migration_when_dedicated_file_exists(tmp_path: Path) -> None: @@ -111,32 +115,39 @@ async def test_async_load_is_idempotent(tmp_path: Path) -> None: assert await asyncio.to_thread(_load_metadata, tmp_path) == shared_after_first -async def test_async_load_recovers_from_corrupt_dedicated_file( +async def test_async_load_preserves_corrupt_dedicated_file( tmp_path: Path, caplog: pytest.LogCaptureFixture ) -> None: - """A corrupt dedicated file falls back to defaults with a warning, no migration.""" + """A corrupt dedicated file is renamed aside (not erased) and logged.""" (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") store = _make_store(tmp_path) - with caplog.at_level("WARNING"): + with caplog.at_level("ERROR"): await store.async_load() assert store.snapshot() == UserPreferences() - assert any("corrupt JSON" in r.message for r in caplog.records) + assert any("undecodable" in r.message for r in caplog.records) + # Original preserved for recovery; the live file is gone so the next save + # can't overwrite a recoverable-but-corrupt read. + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == b"{not valid json" + assert not (tmp_path / _STORE_FILENAME).exists() -async def test_async_load_recovers_from_non_dict_dedicated_file(tmp_path: Path) -> None: - """A dedicated file holding a non-object JSON value falls back to defaults.""" +async def test_async_load_preserves_non_dict_dedicated_file(tmp_path: Path) -> None: + """A dedicated file holding a non-object JSON value is preserved aside.""" (tmp_path / _STORE_FILENAME).write_bytes(b"[1, 2, 3]") store = _make_store(tmp_path) await store.async_load() assert store.snapshot() == UserPreferences() + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == b"[1, 2, 3]" -async def test_async_load_recovers_from_invalid_prefs_shape(tmp_path: Path) -> None: - """A dedicated file whose object fails decode falls back to defaults.""" - (tmp_path / _STORE_FILENAME).write_bytes(b'{"experience_level": "bogus"}') +async def test_async_load_preserves_invalid_shape_dedicated_file(tmp_path: Path) -> None: + """A dedicated file whose object fails decode is preserved aside.""" + raw = b'{"experience_level": "bogus"}' + (tmp_path / _STORE_FILENAME).write_bytes(raw) store = _make_store(tmp_path) await store.async_load() assert store.snapshot() == UserPreferences() + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == raw async def test_round_trip_after_migration(tmp_path: Path) -> None: From 279b72c55d0be0550e7599d9b791dfad8f3533e1 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 22:30:01 -0500 Subject: [PATCH 13/20] Log when preserving a corrupt preferences file fails --- .../controllers/config/_preferences_store.py | 5 +++-- .../config/test_preferences_store.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index 02a0de445..0e3740049 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -5,7 +5,6 @@ import asyncio import logging from collections.abc import Callable -from contextlib import suppress from pathlib import Path from typing import Any @@ -137,8 +136,10 @@ def _snapshot(self) -> UserPreferences: def _preserve_corrupt_file(self) -> None: """Rename the undecodable dedicated file aside so the next save can't erase it.""" path = self._config_dir / _STORE_FILENAME - with suppress(OSError): + try: path.replace(path.with_name(path.name + ".corrupt")) + except OSError: + _LOGGER.warning("Could not preserve corrupt preferences file %s", path, exc_info=True) def _migrate_read_shared_sync(self) -> UserPreferences | None: """Decode the sidecar's ``_preferences`` blob. diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index 40e91b73e..b57ae3038 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -131,6 +131,23 @@ async def test_async_load_preserves_corrupt_dedicated_file( assert not (tmp_path / _STORE_FILENAME).exists() +async def test_preserve_corrupt_file_logs_when_rename_fails( + tmp_path: Path, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """A failed preservation rename is logged, not silently swallowed.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") + + def _boom(self: Path, target: Path) -> None: + raise OSError("disk full") + + monkeypatch.setattr(Path, "replace", _boom) + store = _make_store(tmp_path) + with caplog.at_level("WARNING"): + await store.async_load() + assert store.snapshot() == UserPreferences() + assert any("could not preserve corrupt" in r.message.lower() for r in caplog.records) + + async def test_async_load_preserves_non_dict_dedicated_file(tmp_path: Path) -> None: """A dedicated file holding a non-object JSON value is preserved aside.""" (tmp_path / _STORE_FILENAME).write_bytes(b"[1, 2, 3]") From 4c09b8c79718625cebbe0cab90cf43ab8f0b97c0 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 22:40:07 -0500 Subject: [PATCH 14/20] Gate the sidecar strip on a confirmed migration write --- .../controllers/config/_preferences_store.py | 25 ++++++++++--- .../config/test_preferences_store.py | 35 +++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index 0e3740049..eb3a282ea 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -60,10 +60,9 @@ async def async_load(self) -> None: """Seed RAM from disk; migrate the sidecar's ``_preferences`` on first run. Undecodable data is preserved, never destroyed: a corrupt dedicated file - is renamed aside, and an undecodable legacy sidecar blob is left in place - (not stripped). Both log and fall back to defaults. Migration flushes the - dedicated file before stripping the sidecar key so a crash between the - two preserves the migration. + is renamed aside (then the legacy sidecar is still tried, so a recoverable + blob isn't lost), and an undecodable legacy blob is left in place. Both + fall back to defaults. """ loop = asyncio.get_running_loop() try: @@ -74,16 +73,34 @@ async def async_load(self) -> None: _STORE_FILENAME, ) await loop.run_in_executor(None, self._preserve_corrupt_file) + await self._migrate_from_sidecar(loop) return if loaded is not None: self._state = loaded return + await self._migrate_from_sidecar(loop) + + async def _migrate_from_sidecar(self, loop: asyncio.AbstractEventLoop) -> None: + """Adopt the legacy ``_preferences`` blob, persist it, then strip the key. + + The strip is gated on a confirmed dedicated-file write: ``Store`` logs and + swallows write errors, so an unconfirmed flush leaves the sidecar key in + place for the next start to retry rather than losing prefs on restart. + """ migrated = await loop.run_in_executor(None, self._migrate_read_shared_sync) if migrated is None: return self._state = migrated self._store.async_delay_save(self._snapshot, delay=0.0) await self._store.async_save_now() + if not self._store.path.exists(): + _LOGGER.warning( + "preferences store: %s write unconfirmed; keeping %s in %s to retry", + _STORE_FILENAME, + _PREFS_KEY, + _SHARED_SIDECAR_FILENAME, + ) + return await loop.run_in_executor(None, self._migrate_strip_shared_sync) _LOGGER.info( "Migrated preferences from %s to %s", _SHARED_SIDECAR_FILENAME, _STORE_FILENAME diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index b57ae3038..46a9c130b 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -76,6 +76,41 @@ async def test_async_load_leaves_corrupt_sidecar_blob_in_place(tmp_path: Path) - assert shared["_preferences"] == [1, 2, 3] +async def test_async_load_keeps_sidecar_when_migration_write_unconfirmed( + tmp_path: Path, caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch +) -> None: + """A failed dedicated-file write during migration leaves the sidecar key to retry.""" + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + def _boom(*_a: object, **_k: object) -> None: + raise OSError("disk full") + + monkeypatch.setattr("esphome_device_builder.helpers.storage.atomic_write", _boom) + store = _make_store(tmp_path) + with caplog.at_level("WARNING"): + await store.async_load() + + assert not (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == _SAMPLE.to_dict() + assert any("write unconfirmed" in r.message for r in caplog.records) + + +async def test_async_load_corrupt_dedicated_file_recovers_from_sidecar(tmp_path: Path) -> None: + """A corrupt dedicated file is preserved aside, then prefs recover from the sidecar.""" + (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + store = _make_store(tmp_path) + await store.async_load() + + assert store.snapshot() == _SAMPLE + assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == b"{not valid json" + assert (tmp_path / _STORE_FILENAME).exists() + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert "_preferences" not in shared + + async def test_async_load_skips_migration_when_dedicated_file_exists(tmp_path: Path) -> None: """An existing dedicated file wins; the sidecar ``_preferences`` is left alone.""" new = UserPreferences(theme=Theme.LIGHT) From 25c3ee52540fdf4330a62d72f7d7bb3676b75285 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 22:44:05 -0500 Subject: [PATCH 15/20] Move the migration write-confirmation probe off the event loop --- .../controllers/config/_preferences_store.py | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index eb3a282ea..4cfe1fe27 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -83,9 +83,8 @@ async def async_load(self) -> None: async def _migrate_from_sidecar(self, loop: asyncio.AbstractEventLoop) -> None: """Adopt the legacy ``_preferences`` blob, persist it, then strip the key. - The strip is gated on a confirmed dedicated-file write: ``Store`` logs and - swallows write errors, so an unconfirmed flush leaves the sidecar key in - place for the next start to retry rather than losing prefs on restart. + The strip is gated on a confirmed write so an unconfirmed flush can't lose + the prefs on restart (see :meth:`_confirm_and_strip_shared_sync`). """ migrated = await loop.run_in_executor(None, self._migrate_read_shared_sync) if migrated is None: @@ -93,7 +92,8 @@ async def _migrate_from_sidecar(self, loop: asyncio.AbstractEventLoop) -> None: self._state = migrated self._store.async_delay_save(self._snapshot, delay=0.0) await self._store.async_save_now() - if not self._store.path.exists(): + stripped = await loop.run_in_executor(None, self._confirm_and_strip_shared_sync) + if not stripped: _LOGGER.warning( "preferences store: %s write unconfirmed; keeping %s in %s to retry", _STORE_FILENAME, @@ -101,7 +101,6 @@ async def _migrate_from_sidecar(self, loop: asyncio.AbstractEventLoop) -> None: _SHARED_SIDECAR_FILENAME, ) return - await loop.run_in_executor(None, self._migrate_strip_shared_sync) _LOGGER.info( "Migrated preferences from %s to %s", _SHARED_SIDECAR_FILENAME, _STORE_FILENAME ) @@ -180,7 +179,15 @@ def _migrate_read_shared_sync(self) -> UserPreferences | None: ) return None - def _migrate_strip_shared_sync(self) -> None: - """Drop the migrated ``_preferences`` key from the shared sidecar.""" + def _confirm_and_strip_shared_sync(self) -> bool: + """Strip the migrated ``_preferences`` key, but only if the write landed. + + ``Store`` logs and swallows write errors, so an unconfirmed flush leaves + the sidecar key for the next start to retry. The ``exists`` probe and the + strip share one executor hop to keep the blocking I/O off the loop. + """ + if not self._store.path.exists(): + return False with metadata_transaction(self._config_dir) as data: data.pop(_PREFS_KEY, None) + return True From 762a14737102686137f0717ba87ea0e348fe30b3 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 22:47:34 -0500 Subject: [PATCH 16/20] Return independent copies from update() and mutate() --- .../controllers/config/_preferences_store.py | 7 +++---- .../config/test_preferences_store.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index 4cfe1fe27..359ee15f7 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -117,10 +117,9 @@ def update( self, fields: dict[str, Any], *, delay: float = _DEFAULT_SAVE_DELAY ) -> UserPreferences: """Merge a validated partial dict and schedule a debounced save.""" - new = UserPreferences.from_dict({**self._state.to_dict(), **fields}) - self._state = new + self._state = UserPreferences.from_dict({**self._state.to_dict(), **fields}) self._store.async_delay_save(self._snapshot, delay=delay) - return new + return self._copy() def mutate( self, @@ -140,7 +139,7 @@ def mutate( result = working self._state = result self._store.async_delay_save(self._snapshot, delay=delay) - return result + return self._copy() def _copy(self) -> UserPreferences: """Return a fresh, independent copy of the canonical RAM state.""" diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index 46a9c130b..d54569bcf 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -202,6 +202,23 @@ async def test_async_load_preserves_invalid_shape_dedicated_file(tmp_path: Path) assert (tmp_path / (_STORE_FILENAME + ".corrupt")).read_bytes() == raw +async def test_mutators_return_independent_copies(tmp_path: Path) -> None: + """update()/mutate() return copies; mutating them can't corrupt canonical RAM.""" + store = _make_store(tmp_path) + await store.async_load() + + returned = store.update({"theme": Theme.DARK}) + returned.theme = Theme.LIGHT + assert store.snapshot().theme == Theme.DARK + + def _to_yaml(p: UserPreferences) -> None: + p.experience_level = ExperienceLevel.YAML + + mutated = store.mutate(_to_yaml) + mutated.experience_level = ExperienceLevel.BEGINNER + assert store.snapshot().experience_level == ExperienceLevel.YAML + + async def test_round_trip_after_migration(tmp_path: Path) -> None: """Migrate, mutate, flush, reload: the latest state survives on disk.""" await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) From 97c956a1ea4892ca5741900e3a22045282514005 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 22:50:54 -0500 Subject: [PATCH 17/20] Disable writes when a corrupt prefs file can't be preserved --- .../controllers/config/_preferences_store.py | 29 ++++++++++++++--- .../config/test_preferences_store.py | 31 +++++++++++++++++++ 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/esphome_device_builder/controllers/config/_preferences_store.py b/esphome_device_builder/controllers/config/_preferences_store.py index 359ee15f7..82d0aa40b 100644 --- a/esphome_device_builder/controllers/config/_preferences_store.py +++ b/esphome_device_builder/controllers/config/_preferences_store.py @@ -48,6 +48,9 @@ class PreferencesStore: def __init__(self, config_dir: Path, shutdown_register: ShutdownRegister) -> None: self._config_dir = config_dir self._state = UserPreferences() + # Set when an undecodable file couldn't be renamed aside; suppresses all + # writes so a later save can't overwrite the still-recoverable corrupt file. + self._persist_disabled = False self._store: Store[UserPreferences] = Store( config_dir / _STORE_FILENAME, encoder=_encode, @@ -90,6 +93,8 @@ async def _migrate_from_sidecar(self, loop: asyncio.AbstractEventLoop) -> None: if migrated is None: return self._state = migrated + if self._persist_disabled: + return self._store.async_delay_save(self._snapshot, delay=0.0) await self._store.async_save_now() stripped = await loop.run_in_executor(None, self._confirm_and_strip_shared_sync) @@ -118,7 +123,7 @@ def update( ) -> UserPreferences: """Merge a validated partial dict and schedule a debounced save.""" self._state = UserPreferences.from_dict({**self._state.to_dict(), **fields}) - self._store.async_delay_save(self._snapshot, delay=delay) + self._schedule_save(delay=delay) return self._copy() def mutate( @@ -138,9 +143,15 @@ def mutate( if result is None: result = working self._state = result - self._store.async_delay_save(self._snapshot, delay=delay) + self._schedule_save(delay=delay) return self._copy() + def _schedule_save(self, *, delay: float) -> None: + """Schedule a debounced write, unless persistence has been disabled.""" + if self._persist_disabled: + return + self._store.async_delay_save(self._snapshot, delay=delay) + def _copy(self) -> UserPreferences: """Return a fresh, independent copy of the canonical RAM state.""" return UserPreferences.from_dict(self._state.to_dict()) @@ -149,12 +160,22 @@ def _snapshot(self) -> UserPreferences: return self._state def _preserve_corrupt_file(self) -> None: - """Rename the undecodable dedicated file aside so the next save can't erase it.""" + """Rename the undecodable dedicated file aside so the next save can't erase it. + + If the rename fails, disable persistence: leaving the corrupt file in + place and then writing over it would destroy the recoverable data this + method exists to protect. + """ path = self._config_dir / _STORE_FILENAME try: path.replace(path.with_name(path.name + ".corrupt")) except OSError: - _LOGGER.warning("Could not preserve corrupt preferences file %s", path, exc_info=True) + self._persist_disabled = True + _LOGGER.warning( + "Could not preserve corrupt preferences file %s; disabling writes to keep it", + path, + exc_info=True, + ) def _migrate_read_shared_sync(self) -> UserPreferences | None: """Decode the sidecar's ``_preferences`` blob. diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index d54569bcf..5e95ccf68 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -96,6 +96,37 @@ def _boom(*_a: object, **_k: object) -> None: assert any("write unconfirmed" in r.message for r in caplog.records) +async def test_failed_preserve_disables_writes_to_protect_corrupt_file( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """If the corrupt-file rename fails, writes are suppressed so it isn't clobbered.""" + raw = b"{not valid json" + (tmp_path / _STORE_FILENAME).write_bytes(raw) + await asyncio.to_thread(_save_metadata, tmp_path, {"_preferences": _SAMPLE.to_dict()}) + + real_replace = Path.replace + + def _fail_corrupt_rename(self: Path, target: Path) -> Path: + if str(target).endswith(".corrupt"): + raise OSError("cannot rename") + return real_replace(self, target) + + monkeypatch.setattr(Path, "replace", _fail_corrupt_rename) + store = _make_store(tmp_path) + await store.async_load() + + # RAM recovered from the sidecar for this session, but nothing touches disk. + assert store.snapshot() == _SAMPLE + assert (tmp_path / _STORE_FILENAME).read_bytes() == raw + shared = await asyncio.to_thread(_load_metadata, tmp_path) + assert shared["_preferences"] == _SAMPLE.to_dict() + + # A later write stays suppressed; the recoverable corrupt file is never erased. + store.update({"theme": Theme.LIGHT}) + await store._store.async_save_now() + assert (tmp_path / _STORE_FILENAME).read_bytes() == raw + + async def test_async_load_corrupt_dedicated_file_recovers_from_sidecar(tmp_path: Path) -> None: """A corrupt dedicated file is preserved aside, then prefs recover from the sidecar.""" (tmp_path / _STORE_FILENAME).write_bytes(b"{not valid json") From 2f25f43f0f756b1df9dea2d608b55695756e001e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 13 Jun 2026 22:55:07 -0500 Subject: [PATCH 18/20] Narrow config with an if-guard, not an -O-strippable assert --- esphome_device_builder/device_builder.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/esphome_device_builder/device_builder.py b/esphome_device_builder/device_builder.py index c2ce7f9bd..4fe282863 100644 --- a/esphome_device_builder/device_builder.py +++ b/esphome_device_builder/device_builder.py @@ -603,8 +603,8 @@ async def _send_initial(_controls: StreamControls) -> None: # Gate first-paint UI, so ship them here instead of a separate # get_preferences round-trip. Sync RAM read off the store; the # config controller is always up by the time subscribe is served. - assert self.config is not None - initial["preferences"] = self.config.prefs.snapshot().to_dict() + if self.config is not None: + initial["preferences"] = self.config.prefs.snapshot().to_dict() if self.devices: initial["devices"] = [d.to_dict() for d in self.devices.get_devices()] initial["importable"] = [d.to_dict() for d in self.devices.get_importable_devices()] From a1da4df70b0835c9573591a1d6c04e43a7e74495 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 10:16:41 -0500 Subject: [PATCH 19/20] Collapse experience to BEGINNER/EXPERT and drop yaml_diff_button --- docs/API.md | 4 ++-- esphome_device_builder/controllers/onboarding.py | 10 +++++----- esphome_device_builder/models/preferences.py | 13 ++++++------- tests/controllers/config/test_preferences_store.py | 3 +-- tests/test_onboarding_controller.py | 12 ++++++------ 5 files changed, 20 insertions(+), 22 deletions(-) diff --git a/docs/API.md b/docs/API.md index 804acb953..262f78ce4 100644 --- a/docs/API.md +++ b/docs/API.md @@ -282,7 +282,7 @@ Parsing and writing live on the backend: the frontend exchanges structured `Auto | `config/version` | — | `{server_version, esphome_version}` | Get versions | | `config/serial_ports` | — | `[{port, desc}]` | List serial ports | | `config/get_preferences` | — | `UserPreferences` | Get user preferences | -| `config/set_preferences` | `{theme?, dashboard_view?, experience_level?, remote_compute_only?, ...}` | `UserPreferences` | Update preferences (partial). `experience_level` is `beginner` / `ui` / `yaml` (or `null` until chosen); `remote_compute_only` marks an install as a remote build node. | +| `config/set_preferences` | `{theme?, dashboard_view?, experience_level?, remote_compute_only?, ...}` | `UserPreferences` | Update preferences (partial). `experience_level` is `beginner` / `expert` (or `null` until chosen); `remote_compute_only` marks an install as a remote build node. | | `config/get_secrets` | — | `[string]` | List secret key names | | `config/set_secret` | `{key, value, overwrite?}` | `{created}` | Atomically set one secret in secrets.yaml under a write lock; `overwrite=false` is create-if-absent | @@ -292,7 +292,7 @@ Parsing and writing live on the backend: the frontend exchanges structured `Auto > > Models: [`OnboardingState`, `OnboardingStep`, `OnboardingStepId`, `OnboardingStepStatus`](../esphome_device_builder/models/onboarding.py) -First-run setup tracking. Each step's `status` is computed from live data on every `get_state` call (never persisted), so the frontend's "needs attention" indicators clear the moment the user fixes the underlying state — even via a manual `secrets.yaml` edit. `completed_version` is the last onboarding-flow version the user has explicitly acknowledged; bumping `ONBOARDING_VERSION` (server-side constant) re-prompts users at lower versions when new steps are added. The step list is environment- and preference-aware: `use_case` only on non-HA installs, `wifi_credentials` only when not `remote_compute_only`. A pre-existing install (prior onboarding completed, or device YAML already on disk) is migrated to the `yaml` experience at startup so the wizard never auto-pops for it. +First-run setup tracking. Each step's `status` is computed from live data on every `get_state` call (never persisted), so the frontend's "needs attention" indicators clear the moment the user fixes the underlying state — even via a manual `secrets.yaml` edit. `completed_version` is the last onboarding-flow version the user has explicitly acknowledged; bumping `ONBOARDING_VERSION` (server-side constant) re-prompts users at lower versions when new steps are added. The step list is environment- and preference-aware: `use_case` only on non-HA installs, `wifi_credentials` only when not `remote_compute_only`. A pre-existing install (prior onboarding completed, or device YAML already on disk) is migrated to the `expert` experience at startup so the wizard never auto-pops for it. | Command | Args | Response | Description | |---------|------|----------|-------------| diff --git a/esphome_device_builder/controllers/onboarding.py b/esphome_device_builder/controllers/onboarding.py index 5a090d381..9fa9205f3 100644 --- a/esphome_device_builder/controllers/onboarding.py +++ b/esphome_device_builder/controllers/onboarding.py @@ -92,10 +92,10 @@ async def get_state(self, **kwargs: Any) -> OnboardingState: async def migrate_preexisting_install(self) -> None: """ - Default a pre-existing install to the YAML experience, once. + Default a pre-existing install to the EXPERT experience, once. Installs that completed an earlier onboarding or already hold - device YAMLs predate the experience picker; mark them YAML + device YAMLs predate the experience picker; mark them EXPERT users and acknowledge onboarding so the wizard never auto-pops. Idempotent — a no-op once ``experience_level`` is set. """ @@ -224,7 +224,7 @@ def _compute_state( def _should_migrate_preexisting(prefs: UserPreferences, *, has_device_configs: bool) -> bool: - """Whether a pre-existing install should default to the YAML experience. + """Whether a pre-existing install should default to the EXPERT experience. No-op once an experience is chosen; a fresh install with no device YAMLs and no prior onboarding is left alone so the wizard still runs. @@ -235,8 +235,8 @@ def _should_migrate_preexisting(prefs: UserPreferences, *, has_device_configs: b def _mark_preexisting(p: UserPreferences) -> None: - """Mark *p* a YAML user; acknowledge only if onboarding was already done.""" - p.experience_level = ExperienceLevel.YAML + """Mark *p* an EXPERT user; acknowledge only if onboarding was already done.""" + p.experience_level = ExperienceLevel.EXPERT # Only acknowledge onboarding for installs that already completed it, so a # prior Wi-Fi save or decline is respected. An install known only by its # device YAML stays un-acknowledged, so a missing-Wi-Fi prompt still surfaces. diff --git a/esphome_device_builder/models/preferences.py b/esphome_device_builder/models/preferences.py index 195086244..58853bd2d 100644 --- a/esphome_device_builder/models/preferences.py +++ b/esphome_device_builder/models/preferences.py @@ -34,14 +34,14 @@ class ExperienceLevel(StrEnum): """ How much ESPHome the user knows; tailors UI weight. - Chosen in onboarding, changeable any time in Settings. ``None`` - (a fresh install that hasn't picked) is handled separately; a - pre-existing install migrates to ``YAML``. + Chosen in onboarding, changeable any time via the Settings expert-mode + toggle. ``EXPERT`` unlocks the power-user surfaces (editor diff, navigator + and YAML search). ``None`` (a fresh install that hasn't picked) is handled + separately; a pre-existing install migrates to ``EXPERT``. """ BEGINNER = "beginner" - UI = "ui" - YAML = "yaml" + EXPERT = "expert" @dataclass @@ -58,7 +58,6 @@ class UserPreferences(DataClassORJSONMixin): # Device editor navigator_visible: bool = True - yaml_diff_button: bool = False # Table view settings table_page_size: int = 25 @@ -67,7 +66,7 @@ class UserPreferences(DataClassORJSONMixin): table_sort_direction: SortDirection | None = None # Experience level chosen in onboarding (None = not yet chosen). - # Seeds ``yaml_diff_button`` and the editor's first-open layout. + # ``EXPERT`` unlocks the power-user editor and search surfaces. experience_level: ExperienceLevel | None = None # This install is only a remote build node: onboarding skips the # Wi-Fi step and device-creation entry points are hidden. diff --git a/tests/controllers/config/test_preferences_store.py b/tests/controllers/config/test_preferences_store.py index 3ab605145..99f4b3927 100644 --- a/tests/controllers/config/test_preferences_store.py +++ b/tests/controllers/config/test_preferences_store.py @@ -24,7 +24,6 @@ def _make_store(tmp_path: Path) -> PreferencesStore: _SAMPLE = UserPreferences( navigator_visible=False, - yaml_diff_button=True, theme=Theme.DARK, ) @@ -287,4 +286,4 @@ async def test_round_trip_after_migration(tmp_path: Path) -> None: second = _make_store(tmp_path) await second.async_load() assert second.snapshot().theme == Theme.LIGHT - assert second.snapshot().yaml_diff_button is True + assert second.snapshot().navigator_visible is False diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index ef77266fa..91b78b19b 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -142,7 +142,7 @@ async def test_get_state_experience_set_marks_use_case_and_experience_done( ) -> None: """Picking an experience level completes both leading steps.""" controller = _make_controller( - tmp_path, on_ha_addon=False, prefs=UserPreferences(experience_level=ExperienceLevel.UI) + tmp_path, on_ha_addon=False, prefs=UserPreferences(experience_level=ExperienceLevel.EXPERT) ) state = await controller.get_state() assert _step(state, OnboardingStepId.USE_CASE) == OnboardingStepStatus.DONE @@ -558,7 +558,7 @@ async def test_migrate_acknowledged_install_becomes_yaml_and_stays_acknowledged( controller = _make_controller(tmp_path, prefs=UserPreferences(onboarding_completed_version=1)) await controller.migrate_preexisting_install() prefs = controller._prefs.snapshot() - assert prefs.experience_level == ExperienceLevel.YAML + assert prefs.experience_level == ExperienceLevel.EXPERT assert prefs.onboarding_completed_version == ONBOARDING_VERSION @@ -572,7 +572,7 @@ async def test_migrate_device_yaml_install_stays_unacknowledged(tmp_path: Path) controller = _make_controller(tmp_path) await controller.migrate_preexisting_install() prefs = controller._prefs.snapshot() - assert prefs.experience_level == ExperienceLevel.YAML + assert prefs.experience_level == ExperienceLevel.EXPERT assert prefs.onboarding_completed_version == 0 @@ -582,7 +582,7 @@ async def test_migrate_install_with_yml_extension_becomes_yaml(tmp_path: Path) - controller = _make_controller(tmp_path) await controller.migrate_preexisting_install() prefs = controller._prefs.snapshot() - assert prefs.experience_level == ExperienceLevel.YAML + assert prefs.experience_level == ExperienceLevel.EXPERT assert prefs.onboarding_completed_version == 0 @@ -606,12 +606,12 @@ def test_mark_preexisting_acknowledges_only_a_completed_install() -> None: """The marker sets YAML always, but acknowledges only a completed install.""" completed = UserPreferences(onboarding_completed_version=1) _mark_preexisting(completed) - assert completed.experience_level == ExperienceLevel.YAML + assert completed.experience_level == ExperienceLevel.EXPERT assert completed.onboarding_completed_version == ONBOARDING_VERSION config_only = UserPreferences() _mark_preexisting(config_only) - assert config_only.experience_level == ExperienceLevel.YAML + assert config_only.experience_level == ExperienceLevel.EXPERT assert config_only.onboarding_completed_version == 0 From eac33368315da4dabefe2ce502906915acc4a61d Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 15 Jun 2026 11:15:06 -0500 Subject: [PATCH 20/20] Rename migrate tests to becomes_expert after the BEGINNER/EXPERT collapse --- tests/test_onboarding_controller.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_onboarding_controller.py b/tests/test_onboarding_controller.py index 91b78b19b..0e583a749 100644 --- a/tests/test_onboarding_controller.py +++ b/tests/test_onboarding_controller.py @@ -548,7 +548,7 @@ def test_replace_or_append_secret_value_with_hash_in_quotes_is_misparsed() -> No # --------------------------------------------------------------------------- -async def test_migrate_acknowledged_install_becomes_yaml_and_stays_acknowledged( +async def test_migrate_acknowledged_install_becomes_expert_and_stays_acknowledged( tmp_path: Path, ) -> None: """An install that completed an earlier onboarding keeps its acknowledgement. @@ -563,7 +563,7 @@ async def test_migrate_acknowledged_install_becomes_yaml_and_stays_acknowledged( async def test_migrate_device_yaml_install_stays_unacknowledged(tmp_path: Path) -> None: - """A config-only install that never onboarded gets YAML but no acknowledgement. + """A config-only install that never onboarded gets EXPERT but no acknowledgement. Leaving ``onboarding_completed_version`` at 0 lets a missing-Wi-Fi prompt still fire for these users. @@ -576,7 +576,7 @@ async def test_migrate_device_yaml_install_stays_unacknowledged(tmp_path: Path) assert prefs.onboarding_completed_version == 0 -async def test_migrate_install_with_yml_extension_becomes_yaml(tmp_path: Path) -> None: +async def test_migrate_install_with_yml_extension_becomes_expert(tmp_path: Path) -> None: """``.yml`` is an equally valid config extension; it must trigger migration too.""" (tmp_path / "bedroom.yml").write_text("esphome:\n name: bedroom\n") controller = _make_controller(tmp_path)