Add section config API and user preferences#1
Conversation
- Add ConfigEntry, ConfigValueOption, SectionConfigResponse models for rich
visual editing of YAML sections (core config, components, automations)
- Add section_config module with per-section field definitions and a YAML
parser that extracts current values from device configs
- Add GET/POST /devices/{config}/section-config endpoints to read entries
with current values and update YAML sections from form data
- Add GET/PUT /preferences endpoints for global user preferences (e.g.
editor layout mode), stored in .device-builder.json under _preferences key
- Register new section_config handler routes in server
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds backend support for a “visual section editor” API over ESPHome YAML sections, plus a small preferences API for persisting UI/user settings in the existing .device-builder.json metadata file.
Changes:
- Registers a new
section_confighandler module in the aiohttp app. - Introduces
GET/POST /devices/{configuration}/section-configfor reading/updating “rich” config-entry definitions with values parsed from YAML. - Adds
GET/PUT /preferencesbacked by.device-builder.jsonunder the_preferenceskey, plus new related models.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
server.py |
Registers the new handlers/section_config.py route table. |
section_config.py |
Adds section-entry definitions and a text-based YAML value extractor used by the section-config API. |
models.py |
Adds dataclasses for ConfigEntry, ConfigValueOption, SectionConfigResponse, and UserPreferences. |
metadata.py |
Adds _preferences read/merge-write helpers using .device-builder.json. |
handlers/section_config.py |
Implements GET/POST endpoints and a text-based YAML updater for section values. |
handlers/misc.py |
Implements GET/PUT /preferences endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if result is None: | ||
| return error_response(f"Unknown section: {section_key}", status=404) |
There was a problem hiding this comment.
get_section_config() never returns None (it returns a generic SectionConfigResponse for unknown keys), so this 404 branch is dead and the endpoint behavior is inconsistent. Either update get_section_config() to actually return None for unknown keys, or (per the PR description) remove this check and always return the generic alert response.
| if result is None: | |
| return error_response(f"Unknown section: {section_key}", status=404) |
| return error_response("Invalid JSON body") | ||
|
|
||
| section_key = body.get("section_key", "") | ||
| values: dict[str, Any] = body.get("values", {}) | ||
|
|
||
| if not section_key: | ||
| return error_response("Missing 'section_key'") |
There was a problem hiding this comment.
values is assumed to be a dict but body.get("values") can be any JSON type (e.g. list/null), which will cause _update_yaml_section() to crash when iterating .items() or looking up keys. Validate that values is a dict (and reject non-dict bodies) and return a 400 error when the shape is invalid.
| return error_response("Invalid JSON body") | |
| section_key = body.get("section_key", "") | |
| values: dict[str, Any] = body.get("values", {}) | |
| if not section_key: | |
| return error_response("Missing 'section_key'") | |
| return error_response("Invalid JSON body", status=400) | |
| if not isinstance(body, dict): | |
| return error_response("Invalid JSON body: expected an object", status=400) | |
| section_key = body.get("section_key", "") | |
| raw_values = body.get("values", {}) | |
| if not isinstance(raw_values, dict): | |
| return error_response("Invalid 'values': expected an object", status=400) | |
| values: dict[str, Any] = raw_values | |
| if not section_key: | |
| return error_response("Missing 'section_key'", status=400) |
| # Handle nested parent keys (key with no value, e.g. 'encryption:') | ||
| parent_match = re.match(r"^\s+(\w[\w.]*)\s*:\s*$", line) | ||
| if parent_match: | ||
| current_parent = parent_match.group(1) | ||
| continue |
There was a problem hiding this comment.
current_parent is set when encountering a nested mapping key, but there’s no indentation tracking to know when that nested block ends. This will incorrectly treat sibling keys (e.g. port: under api:) as children of the previous parent (e.g. encryption.port), leading to wrong reads and writes. Track the parent indentation level and reset current_parent when indentation decreases back to the section child level (or switch to a YAML parser for value extraction).
| # Handle list items (- platform: gpio) | ||
| list_match = re.match(r"^(\s*)-\s+(\w[\w.]*)\s*:\s*(.+)$", line) | ||
| if list_match: | ||
| key = list_match.group(2) | ||
| val = list_match.group(3).strip() |
There was a problem hiding this comment.
List sections (e.g. sensor: / switch:) can have multiple items; this parser stores list item keys into a flat dict, so repeated keys overwrite earlier items and only the last item “wins”. If this endpoint is meant to support component sections, you’ll need a way to address a specific list item (e.g. by index/id/name) and return/update per-item values instead of collapsing the whole list into one dict.
| parent_match = re.match(r"^(\s+)(\w[\w.]*)\s*:\s*$", line) | ||
| if parent_match: | ||
| current_parent = parent_match.group(2) | ||
| base_indent = parent_match.group(1) | ||
| continue |
There was a problem hiding this comment.
Same indentation issue as the reader: current_parent is never cleared based on indentation, so after a nested block like encryption:, subsequent sibling keys may be treated as encryption.<key> and updated/added under the wrong path. Track the parent indent level and reset current_parent when encountering a key at the parent’s indent (or lower).
| # Handle dotted keys (e.g. 'encryption.key') | ||
| if "." in key: | ||
| parent, child = key.rsplit(".", 1) | ||
| append_lines.append(f"{base_indent}{parent}:\n") | ||
| append_lines.append(f"{base_indent} {child}: {_format_yaml_value(val)}\n") |
There was a problem hiding this comment.
For dotted keys, this appends a new parent: line for every key (e.g. encryption.key and encryption.foo will produce duplicate encryption: blocks). It also doesn’t check whether the parent mapping already exists in the section. Consider grouping appended dotted keys by parent and emitting a single parent block, or inserting missing children into an existing parent block when present.
| list_match = re.match(r"^(\s*-\s+)(\w[\w.]*)\s*:\s*(.*)$", line) | ||
| if list_match: | ||
| prefix = list_match.group(1) | ||
| key = list_match.group(2) | ||
| current_parent = "" |
There was a problem hiding this comment.
List item updates are applied purely by key name; if a section contains multiple list items (multiple sensors/switches), this loop will update every matching - <key>: occurrence, unintentionally modifying multiple components at once. The update API likely needs a way to target a specific list entry (index/id/name) rather than bulk-updating all list items with the same key.
| return error_response("Invalid JSON body") | ||
|
|
There was a problem hiding this comment.
request.json() can return any JSON type; if the client sends a list/string/etc, set_preferences() will raise (because it expects a mapping). Add a check that body is a dict and return a 400 error when it’s not (and ideally validate allowed preference keys/values).
| return error_response("Invalid JSON body") | |
| return error_response("Invalid JSON body", status=400) | |
| if not isinstance(body, dict): | |
| return error_response("Invalid JSON body: expected an object", status=400) |
| def set_preferences(config_dir: Path, prefs: dict[str, Any]) -> dict[str, Any]: | ||
| data = _load(config_dir) | ||
| current = data.setdefault(_PREFS_KEY, {}) | ||
| current.update(prefs) | ||
| _save(config_dir, data) |
There was a problem hiding this comment.
current.update(prefs) assumes prefs is a dict and that the stored _preferences value is also a dict. If either is not a mapping (e.g. malformed JSON, or caller passes a list), this will raise and break the /preferences endpoint. Add defensive type checks/coercion (e.g. treat non-dict _preferences as {} and reject non-dict prefs with a clear error).
Address Copilot review on PR #105: 1. _archive_single now wipes the StorageJSON sidecar AND the device-metadata entry alongside the build dir. Previously left them in place so unarchive could restore cached state (address / version / hash / loaded_integrations), but per-filename keying means a future same-name configuration would inherit the archived device's stale state until recompiled or edited. Trade a few seconds of "unknown state" after unarchive (the scanner + monitor refill from the next mDNS broadcast) for full isolation between archive and any future same-name device. Updated docstrings to reflect. 2. _delete_archived_single now skips sidecar removal when an active config of the same filename exists. Defensive — with #1 in place this collision shouldn't arise, but if the archive predates this PR (or was written by the legacy dashboard which kept sidecars), the StorageJSON / metadata belong to the live device and removing them would wipe its cached IP / hash / loaded_integrations. Path-traversal verification: 11 traversal payloads x 3 archive commands all still rejected with INVALID_ARGS. The new code paths (sidecar wipe in archive, active-config check in delete_archived) all run after _validate_archive_configuration at the public boundary; the metadata file is hardcoded (.device-builder.json) and configuration is used only as a dict key. Tests: 33 archive-specific (3 new), 515 backend total.
The orphan was the actual bug, not real DNS. The previous commit's test patches were the right shape (avoid network in tests), but they masked the underlying lifecycle race — which is why the same hang resurfaced under pytest-timeout on Linux + Python 3.14 + non-stable esphome. Race timeline (real or stubbed network — same shape either way): * request_pair #1 spawns listener_v1, dict[key] = v1. * listener_v1 runs to its 'await peer_link_await_pair_status' parked state. * request_pair #2 calls _cancel_pair_status_listener (pops v1, cancels) then _spawn_pair_status_listener (dict[key] = v2). * The test awaits listener_v1; the loop wakes v1 with CancelledError at the parked await. v1 enters its finally, which unconditionally popped dict[key] — but the slot now holds v2. v2 is silently evicted from the listener registry. * unpair calls _cancel_pair_status_listener — dict empty, no-op. v2 is orphaned, parked forever; 'await listener_v2' hangs (wedged event loop, selector timeout=-1, no scheduled work — exactly what the failing pytest-timeout dump showed). Locally the bug was masked because v1 typically gets cancelled while still parked at run_in_executor (its initial identity load), so the cancel raises before the body's try block — v1's finally never runs and never evicts v2. On Linux + Python 3.14 + beta/dev esphome the scheduling order was different: v1 had time to advance to peer_link_await_pair_status before being cancelled, the finally fired, the eviction happened, the test hung. Fix: only clear the dict slot if it still points at the current task. asyncio.current_task() compared against the stored task is the canonical 'am I still the registered listener for this key' check. Successor v2 stays in the registry, unpair cancels it cleanly, no orphan. Test: forced the race deterministically. The fake peer_link_await_pair_status now sets a per-call asyncio.Event when it parks; the test waits on the first such event before the second request_pair so v1 is guaranteed to be in the critical section when cancellation arrives. Verified that the test deterministically hangs without the controller fix (8s timeout fires) and passes in 0.04s with it. The full remote_build test suite (184 tests) is green.
* On-disk validation of cleanup_ttl_seconds (Copilot). The WS validator on set_settings gates write-through-WS, but the on-disk decode path didn't apply the same not_bool / range check. A sidecar with cleanup_ttl_seconds: true would deserialise as 1 (bool is an int subclass) and the sweep would treat anything older than 1s as cold. Other wrong types (string, float, None) would propagate to the sweep's now - ttl_seconds arithmetic and raise TypeError every cycle. New __post_init__ on RemoteBuildSettings coerces non-int / bool back to DEFAULT_CLEANUP_TTL_SECONDS and clamps the result to [MIN, MAX]. Doesn't reject the row (no ValueError) — the load path stays robust against partially-corrupt sidecars; the operator's last-good enabled toggle survives even if the TTL field is broken. Six new tests in test_config_controller.py pin the coerce-to-default branches, the clamp-to-MIN / clamp-to-MAX branches, and the pass-through-on-valid branch. * Public seam on FirmwareController for in-flight remote-peer jobs (reviewer concern #1). Reaching into firmware._jobs from the cleanup loop coupled the two controllers to the private dict shape — a future refactor (lock-wrapped jobs, QUEUED + RUNNING split into two dicts, indexed view) would silently break this loop. New FirmwareController.active_remote_peer_jobs generator filters to QUEUED / RUNNING + non-empty remote_peer. The cleanup loop now consumes through that seam. Two new tests pin every filter branch (local skip, remote queued / running yielded, remote completed skipped, empty case). * Named constant for the parse_from_configuration tail-segment count (reviewer nit). The +3 magic was folklore; replaced with _TAIL_SEGMENT_COUNT and a docstring explaining the three required tail parts (dashboard_id, device_name, YAML filename under the device subtree). Reviewer concern #2 (RAM-canonical settings) and #4 (rate-limit per cycle) deferred per their "worth a small follow-up" framing. Concern #3 (first-sweep latency) was considered but YAGNI for this PR: the receiver-side cleanup ships ahead of any production user (alpha, ~10 beta testers), so no accumulated subtrees exist before this code's first cycle — and the default 24h TTL means even a fresh receiver has nothing to reclaim for the first day regardless.
Summary
GET/POST /devices/{config}/section-config): Returns richConfigEntrylists for any YAML section (core config like wifi/api/logger, components like sensor/switch/light, automations like script/interval) with current values parsed from the device YAML. POST updates values back into the YAML preserving formatting.GET/PUT /preferences): Global user preferences (e.g. editor layout mode) stored in.device-builder.jsonunder a_preferenceskey. Supports partial merge updates.ConfigEntry,ConfigValueOption,SectionConfigResponse,UserPreferencesTest plan
GET /devices/{config}/section-config?key=wifireturns entries with current values from YAMLGET /devices/{config}/section-config?key=sensorreturns component entries with parsed valuesPOST /devices/{config}/section-configwith{section_key, values}updates YAML correctlyGET /preferencesreturns{}initially, saved preferences after PUTPUT /preferenceswith{editor_layout: "left"}persists and returns updated prefs🤖 Generated with Claude Code