Correct device-removal #940
Conversation
WalkthroughReplaces set-based device ID tracking with DeviceEntry-based tracking and device-registry lookups; diffs registry-derived IDs against discovered IDs to compute additions/removals; changes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant HA as Home Assistant
participant Coord as PlugwiseCoordinator
participant DR as DeviceRegistry
participant GW as Gateway
HA->>Coord: Trigger device sync (_async_add_remove_devices)
Coord->>DR: async_entries_for_config_entry(config_entry)
DR-->>Coord: List[DeviceEntry]
Note over Coord: Build _current_devices (list of DeviceEntry) and current_ids from identifiers
Coord->>GW: Obtain discovered device IDs
GW-->>Coord: discovered IDs
rect rgba(200,230,255,0.25)
Note over Coord: Compute deltas
Coord->>Coord: new_devices = discovered_ids - current_ids
Coord->>Coord: removed_ids = current_ids - discovered_ids
end
alt Removed devices present
Coord->>DR: async_get_device(via_device_identifiers)
DR-->>Coord: gateway_device
Coord->>Coord: find device entries with via_device_id == gateway_device.id and identifiers ∩ removed_ids
Coord->>DR: device_registry.async_update_device(device_id, remove_config_entry_id=entry_id)
DR-->>Coord: DeviceEntry updated (association removed)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Prepare and validate pre-commit
🔇 Additional comments (3)
custom_components/plugwise/coordinator.py (3)
28-28: LGTM!The
DeviceEntryimport is correctly added to support the new device tracking implementation.
77-78: Type changes align with new DeviceEntry-based tracking.The type changes from
set[str]tolist[DeviceEntry]for_current_devicesand fromset[str]tolist[str]fornew_devicesare appropriate for the new device tracking approach using Home Assistant's device registry.
136-139: LGTM!Correctly initializes the device registry reference and populates
_current_deviceswith the current device entries for this config entry.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
bouwew
force-pushed
the
improve-delete
branch
from
f77bf82 to
5ca26a9
Compare
bouwew
force-pushed
the
improve-delete
branch
from
acd2d49 to
46aa590
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup for HA-core (release/master)
- GitHub Check: Setup for HA-core (dev)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
custom_components/plugwise/coordinator.py(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Validate with hassfest
custom_components/plugwise/coordinator.py
[error] 161-161: IndentationError: unindent does not match any outer indentation level.
🪛 Ruff (0.14.0)
custom_components/plugwise/coordinator.py
161-161: unindent does not match any outer indentation level
(invalid-syntax)
162-162: Unexpected indentation
(invalid-syntax)
bouwew
force-pushed
the
improve-delete
branch
from
072a21e to
3007cb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
custom_components/plugwise/coordinator.py (4)
136-147: Compute IDs once, sort new_devices, and pass removed_ids to the removerSlightly simplify and make ordering deterministic; also pass only the needed IDs to the remover.
- self.device_registry = dr.async_get(self.hass) - self._current_devices = dr.async_entries_for_config_entry( - self.device_registry, self.config_entry.entry_id - ) - current_device_ids = { + self.device_registry = dr.async_get(self.hass) + self._current_devices = dr.async_entries_for_config_entry( + self.device_registry, self.config_entry.entry_id + ) + data_ids = set(data) + current_device_ids = { identifier[1] for device_entry in self._current_devices for identifier in device_entry.identifiers if identifier[0] == DOMAIN } - self.new_devices = list(set(data) - current_device_ids) - removed_devices = list(current_device_ids - set(data)) + self.new_devices = sorted(data_ids - current_device_ids) + removed_ids = current_device_ids - data_ids @@ - if removed_devices: - await self._async_remove_devices(data) + if removed_ids: + await self._async_remove_devices(removed_ids)Also applies to: 149-151
152-152: Narrow removal loop to removed_ids and avoid duplicate updatesPass removed_ids and break after the first identifier match to prevent redundant updates.
- async def _async_remove_devices(self, data: dict[str, GwEntityData]) -> None: + async def _async_remove_devices(self, removed_ids: set[str]) -> None: @@ - via_device_id = gateway_device.id - for device_entry in self._current_devices: - for identifier in device_entry.identifiers: - if ( - identifier[0] == DOMAIN - and device_entry.via_device_id == via_device_id - and identifier[1] not in data - ): - self.device_registry.async_update_device( - device_entry.id, remove_config_entry_id=self.config_entry.entry_id - ) - LOGGER.debug( - "Removed %s device/zone %s %s from device_registry", - DOMAIN, - device_entry.model, - identifier[1], - ) + via_device_id = gateway_device.id + for device_entry in self._current_devices: + if device_entry.via_device_id != via_device_id: + continue + for identifier in device_entry.identifiers: + if identifier[0] == DOMAIN and identifier[1] in removed_ids: + self.device_registry.async_update_device( + device_entry.id, remove_config_entry_id=self.config_entry.entry_id + ) + LOGGER.debug( + "Removed %s device/zone %s %s from device_registry", + DOMAIN, + device_entry.model, + identifier[1], + ) + breakAlso applies to: 161-171
136-139: Avoid fetching the device registry on every updateCache the registry in init to avoid repeated lookups.
Example outside this hunk:
# __init__ self.device_registry = dr.async_get(hass)Then drop the assignment in _async_add_remove_devices.
77-79: Sort new_devices for deterministic ordering
new_devices is used across multiple _add_entities methods; replace the unorderedlist(set(data) - current_device_ids)with a sorted list (e.g.self.new_devices = sorted(set(data) - current_device_ids) ```) to ensure consistent ordering. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between b84fbf1b1a40f71faf209c850712b18b946286f1 and 3007cb1744c1293ade15e4659441b162e1e9ea90. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `custom_components/plugwise/coordinator.py` (3 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 GitHub Actions: Test PR against HA-core</summary> <details> <summary>custom_components/plugwise/coordinator.py</summary> [error] 156-156: Ruff: 1 issue found and fixed by pre-commit hook; process exited with code 1 due to changes. Please commit again after reviewing the changes. </details> </details> </details> <details> <summary>🔇 Additional comments (1)</summary><blockquote> <details> <summary>custom_components/plugwise/coordinator.py (1)</summary><blockquote> `155-158`: **Gateway guard and logger usage look good** Properly handles missing gateway and uses LOGGER correctly. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
bouwew
force-pushed
the
improve-delete
branch
from
3007cb1 to
0608749
Compare
bouwew
force-pushed
the
improve-delete
branch
from
258fc78 to
89f4ee3
Compare
|
|
No, this doesn't work :( |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores