Conversation
Collaborator
stvncode
commented
Apr 2, 2026
stvncode
commented
- .components.dashboard_import fix the problem when trying to run the servor
- and the removal of the to_dict an error server error when trying to reach the catalog in the fe
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates backend handlers to return proper aiohttp.web.Response objects for catalog-style endpoints and adjusts the ESPHome import_config import to a path that matches newer ESPHome layouts.
Changes:
- Remove erroneous
.to_dict()calls onjson_response(...)results for section/config catalog endpoints. - Update
import_configimport toesphome.components.dashboard_importto fix server startup/import issues.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
esphome_device_builder/handlers/section_config.py |
Fixes the section-config GET endpoint to return a web.Response (avoids calling .to_dict() on a response). |
esphome_device_builder/handlers/devices.py |
Updates the import_config import path to align with ESPHome’s dashboard_import module. |
esphome_device_builder/handlers/config_sections.py |
Fixes the config catalog endpoint to return a web.Response (avoids calling .to_dict() on a response). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4 tasks
bdraco
added a commit
that referenced
this pull request
May 2, 2026
- ``DeviceScanner.get_by_name`` now returns a fresh list snapshot. Previously the method handed out a reference to the live bucket; a misbehaving caller could mutate it (``.clear()``, ``.sort()``) and quietly poison the name index for future lookups. Matches the semantics of the ``devices`` property. - ``_set_device`` keeps each bucket sorted by ``configuration`` filename. Insertion order used to depend on ``paths_to_load`` (a set), so ``bucket[0]`` consumers — including ``_find_device_by_name`` and the apply / dedupe path — saw a non-deterministic "first match" for duplicate-named YAMLs and could flip-flop across scans. - ``apply()`` now dedupes against every matching device's state, not just the first. With two YAMLs sharing an ``esphome.name``, one sibling could be in-sync while the other was rebuilt with state=UNKNOWN; the old "first device matches → bail" path left the stale sibling stuck. - Reworded the priority-2 docstring in ``compute_has_pending_changes`` to reflect the actual condition (no comparable hash pair AND no local binary), since with the reorder #2 can fire even when one hash side has been broadcast. - New regression tests: - ``test_get_by_name_returns_a_snapshot`` - ``test_index_bucket_order_is_deterministic`` - ``test_apply_state_repairs_stale_sibling_when_first_match_is_in_sync``
bdraco
added a commit
that referenced
this pull request
May 7, 2026
Three real bugs Copilot flagged in the YAML rewriter and clone
guards. Fix all three; add coverage for each.
**1. ``rewrite_yaml_scalar`` matched off-path nested keys.**
The walker only pushed *on-path* mapping keys onto its ancestor
stack. For path ``("api", "encryption", "key")``, YAML like
``api: { something: { encryption: { key: ... } } }`` would
falsely satisfy the path comparison because ``something`` was
invisible to the ancestor check. Push every mapping key (on-path
or not) so off-path branches show up in the ancestor chain and
the comparison fails correctly.
**2. ``.yaml`` vs ``.yml`` extension slipped past the same-name
guard.** ``configuration="kitchen.yml"`` + ``new_name="kitchen"``
produced ``new_filename="kitchen.yaml"``, which doesn't equal the
configuration string, so the same-name check passed — but both
files would carry ``esphome.name: kitchen`` and collide on mDNS
once the clone was flashed. Compare on stems instead of filenames.
**3. Friendly names with YAML-special characters got silently
truncated / split.** A friendly name like ``Bedroom #2`` written
as a plain scalar would round-trip as just ``Bedroom`` (everything
after `` #`` becomes a YAML comment); ``Lamp: Kitchen`` would
split into a key/value pair. Add ``_safe_yaml_scalar`` which emits
plain scalars when safe and double-quoted otherwise. Route
``rewrite_name_or_substitution`` and the encryption-key rewrite
through it.
DRY pass:
- Extract ``_strip_yaml_quotes`` shared by ``rewrite_esphome_name``
and ``parse_substitution_ref``.
- Drop the unused ``rewrite_friendly_name`` (clone now goes
through ``rewrite_name_or_substitution``).
22 new / updated tests pin the off-path soundness fix, the
``.yml`` collision rejection, the safe-quoting matrix, and the
``_strip_yaml_quotes`` helper. 1811/1811 backend tests pass.
bdraco
added a commit
that referenced
this pull request
May 7, 2026
Closes esphome/feature-requests#2868. The use case that drives this: someone gets a Made-For-ESPHome device, adopts it through the dashboard, and now wants to call it "Reading Lamp" instead of the manufacturer's default friendly_name. They don't know YAML yet — opening the editor and finding ``esphome:`` -> ``friendly_name:`` is a wall they shouldn't have to scale just to rename their bulb. The existing ``devices/update`` only writes the metadata sidecar JSON, not the YAML, so wiring a UI to it would let the dashboard label drift from what the running firmware broadcasts. The new command rewrites the YAML directly via the same ``rewrite_name_or_substitution`` machinery the clone path is built on — substitution-aware (``friendly_name: ${friendly_name}`` redirects through the substitutions block) and safe on YAML-special characters (``Bedroom #2`` round-trips through ``_safe_yaml_scalar`` quoting). The install half stays out of this command. The frontend already owns the OTA UX (opens the command-dialog, follows the streaming job) so a separate ``firmware/install`` call after this one composes naturally with the existing rename / install paths. DRY pass: the "leaf must exist in this file" precondition + the rewrite were inlined in clone_device. Extracted into a shared ``_rewrite_required_yaml_leaf`` helper so the pattern lives in one place. Eight new tests cover the literal-leaf rewrite, substitution redirect, YAML-special quoting, idempotent no-op, all three INVALID_ARGS paths, and lookalike-line preservation.
bdraco
added a commit
that referenced
this pull request
May 9, 2026
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.
16 tasks
bdraco
added a commit
that referenced
this pull request
May 10, 2026
Three fixes from the latest Copilot pass: * `_dispatch_submit_job_ack` only copies `reason` through when `accepted=False`. `SubmitJobAckFrameData.reason` is `NotRequired` and means "rejection code on negative ack"; the previous shape would have propagated a spurious `reason` on a positive ack if the receiver ever included one off-contract. Now drops + debug-logs the stray field instead. * `_send_submit_job_frames` streams chunks via `chunk_bundle`'s generator instead of materialising the full list up front. `num_chunks` is computed via integer ceil on `total_bundle_bytes` so the header still announces the exact count without holding every slice alive simultaneously. Cuts peak memory roughly in half on the bundle hot path (up to `BUNDLE_MAX_TOTAL_BYTES`, 4 MiB). * `_validate_submit_job_config` now returns `(name, yaml_path)` so the downstream `_build_submit_job_bundle` reuses the already-resolved `Path` instead of calling `rel_path` again. Drops a duplicate blocking `Path.resolve` syscall on every submit.
bdraco
added a commit
that referenced
this pull request
May 11, 2026
* 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.
This was referenced May 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.