Fire OFFLOADER_PAIRING_ADDED on request_pair for cross-tab sync#1211
Draft
esphbot wants to merge 3 commits into
Draft
Fire OFFLOADER_PAIRING_ADDED on request_pair for cross-tab sync#1211esphbot wants to merge 3 commits into
esphbot wants to merge 3 commits into
Conversation
Merging this PR will not alter performance
Comparing 
Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1211 +/- ##
=======================================
Coverage 99.42% 99.42%
=======================================
Files 217 217
Lines 16140 16160 +20
=======================================
+ Hits 16047 16067 +20
Misses 93 93
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Draft
13 tasks
Contributor
Author
|
Frontend half (consumes this event): esphome/device-builder-frontend#637 |
Member
|
Looks spot on . real gap |
Member
|
@esphbot rr |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an offloader-side bus event to keep pairing lists consistent across multiple connected clients (e.g., multiple browser tabs) by emitting a “row created” event when request_pair creates/recreates a pairing.
Changes:
- Introduces
EventType.OFFLOADER_PAIRING_ADDEDand itsOffloaderPairingAddedDatapayload. - Fires
OFFLOADER_PAIRING_ADDEDfromrequest_pairfor both PENDING and short-circuit APPROVED outcomes. - Adds an end-to-end test asserting the event is emitted from the offloader pairing flow.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_remote_build_peer_link_client.py |
Adds a new test ensuring request_pair emits the pairing-added event. |
esphome_device_builder/models/remote_build/offloader_events.py |
Defines the new OffloaderPairingAddedData TypedDict payload. |
esphome_device_builder/models/remote_build/__init__.py |
Re-exports OffloaderPairingAddedData. |
esphome_device_builder/models/common.py |
Adds EventType.OFFLOADER_PAIRING_ADDED. |
esphome_device_builder/controllers/remote_build/pair_status.py |
Adds a helper to build/fire the OFFLOADER_PAIRING_ADDED payload. |
esphome_device_builder/controllers/remote_build/pair_commands.py |
Fires the new event from request_pair using the existing _pairing_summary_for projection. |
esphome_device_builder/controllers/remote_build/offloader.py |
Adds a thin controller method delegating to the new fire helper. |
Comment on lines
+842
to
+862
| await offloader.request_pair( | ||
| hostname="127.0.0.1", | ||
| port=server.port, | ||
| pin_sha256=expected_pin, | ||
| receiver_label="my-receiver", | ||
| offloader_label="my-builder", | ||
| ) | ||
|
|
||
| added = [ | ||
| call.args[1] | ||
| for call in offloader._db.bus.fire.call_args_list | ||
| if call.args[0] is EventType.OFFLOADER_PAIRING_ADDED | ||
| ] | ||
| assert len(added) == 1 | ||
| payload = added[0] | ||
| assert payload["pin_sha256"] == expected_pin | ||
| assert payload["receiver_hostname"] == "127.0.0.1" | ||
| assert payload["receiver_port"] == server.port | ||
| assert payload["label"] == "my-receiver" | ||
| assert payload["status"] == "pending" | ||
| assert payload["enabled"] is True |
Comment on lines
+35
to
+50
| class OffloaderPairingAddedData(TypedDict): | ||
| """ | ||
| Payload for ``EventType.OFFLOADER_PAIRING_ADDED``. | ||
|
|
||
| Fires from ``request_pair`` when a :class:`StoredPairing` row | ||
| is created (or re-created on a re-pair). ``OFFLOADER_PAIR_STATUS_CHANGED`` | ||
| only marks status *flips* of an already-known row, so without | ||
| this event a second connected tab never learns of a pairing | ||
| the originating tab just made (it seeds its own row from the | ||
| command response) — cross-tab desync until reload. | ||
|
|
||
| Carries every :class:`PairingSummary` field so a subscriber | ||
| builds the row from the event alone. ``status`` is | ||
| ``"pending"`` for a fresh request or ``"approved"`` when the | ||
| receiver short-circuited the inbox dance on a re-pair. | ||
| """ |
Contributor
Author
PR Review — Fire OFFLOADER_PAIRING_ADDED on request_pair for cross-tab syncSolid, well-scoped fix for a real cross-tab desync gap. Merge-ready; one docstring nit.
🟢 Suggestions1. TypedDict docstring carries motivation CLAUDE.md asks to omit (`esphome_device_builder/models/remote_build/offloader_events.py`, L35-53)Per the repo's code-style rule (CLAUDE.md: "Never put in docstrings or comments: rationale / motivation"), this docstring leans heavily into why — the cross-tab desync narrative, what a second tab does, the seed-from-command-response detail. That belongs in the PR body (where it already is), not the payload contract. Trim to the contract: what the event signals and what class OffloaderPairingAddedData(TypedDict):
"""
Payload for ``EventType.OFFLOADER_PAIRING_ADDED`` (a row created by
``request_pair``). Carries every ``PairingSummary`` field so a
subscriber builds the row from the event alone. ``status`` is
``"pending"`` for a fresh request, ``"approved"`` on a re-pair
short-circuit.
"""Matches the Copilot reviewer's note on the same lines. Non-blocking. Automated review by Kōan (Claude) |
request_pair created a StoredPairing row but fired no bus event, so a second connected tab never learned of a pairing the originating tab made (it seeds its own row from the command response). The later OFFLOADER_PAIR_STATUS_CHANGED only flips an already-known row, so the second tab's row stayed absent until a full reload. Add EventType.OFFLOADER_PAIRING_ADDED + OffloaderPairingAddedData carrying the full PairingSummary, fired from request_pair for both the PENDING and re-pair-APPROVED branches, mirroring the receiver-side REMOTE_BUILD_PAIR_REQUEST_RECEIVED row-create event.
Contributor
Author
Rebase with requested adjustmentsBranch Changes applied
StatsActions performed
CI statusCI will be checked asynchronously. Automated by Kōan |
esphbot
force-pushed
the
koan/offloader-pairing-added-event
branch
from
413a6db to
5fd418f
Compare
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.
What
Fire a new
OFFLOADER_PAIRING_ADDEDbus event whenrequest_paircreates an offloader pairing, so connected clients that didn't issue the command build the row from the event.Why
request_pairpersisted theStoredPairingand returned aPairingSummaryin the command response, but fired no bus event for the creation. The originating tab seeds its own row from that response (see the frontendonPairRequestSent), but a second connected tab gets nothing — and the laterOFFLOADER_PAIR_STATUS_CHANGEDonly flips an already-known row (patchOffloadPairingno-ops on a missing key). So a second tab's pairings list stayed stale until a full reload. This is the snapshot-plus-events cross-tab desync the architecture explicitly avoids (same vein as #1188 / #1189).How
EventType.OFFLOADER_PAIRING_ADDED+OffloaderPairingAddedDataTypedDict carrying everyPairingSummaryfield, so a subscriber builds the row from the event alone — mirroring the receiver-sideREMOTE_BUILD_PAIR_REQUEST_RECEIVED.request_pairfor both the PENDING and the re-pair-short-circuit APPROVED branches, off the single-source-of-truth_pairing_summary_forprojection.device-builder-frontend) to insert the row.Testing
test_controller_request_pair_fires_pairing_added_event(end-to-end through the WS-command shell) asserts the event fires once with the full payload.pytest tests/test_remote_build_peer_link_client.py tests/test_remote_build_controller.py tests/test_subscribe_events_cleanup.py tests/models→ 426 passed. ruff + mypy clean.Types of changes
bugfixFrontend coordination
Quality Report
Changes: 7 files changed, 106 insertions(+), 3 deletions(-)
Code scan: clean
Tests: failed ([Errno 13] Permission denied: 'pytest')
Branch hygiene: clean
Generated by Kōan