Conversation
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds support for resolving room names for shared/received devices by introducing a dedicated web API endpoint and wiring it into the V1 Rooms trait logic (so shared devices don’t rely on the owner’s home room list).
Changes:
- Add
get_shared_device_roomsendpoint to the web API client (with response normalization forroomIdvsid). - Update
RoomsTraitto use shared-device rooms when the device is detected as shared, without mutatingHomeData.rooms. - Add test coverage for the new endpoint and shared-device room name refresh behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
roborock/web_api.py |
Adds get_shared_device_rooms to RoborockApiClient and UserWebApiClient. |
roborock/devices/traits/v1/rooms.py |
Uses shared-room lookup path for shared devices and maintains a separate shared room-name map. |
roborock/devices/traits/v1/__init__.py |
Passes device_uid into RoomsTrait so it can detect shared devices. |
tests/test_web_api.py |
Adds parametrized test for the new shared-device rooms endpoint payload variants. |
tests/devices/traits/v1/test_rooms.py |
Adds test ensuring shared device room names come from get_shared_device_rooms and don’t mutate HomeData.rooms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @cached_property | ||
| def _is_shared(self) -> bool: | ||
| return any(d.duid == self._device_uid for d in self._home_data.received_devices) | ||
|
|
There was a problem hiding this comment.
I think it's a pretty big edge case. It's very unlikely that shared devices changes mid run and we don't even really support that right now
But defer to your thoughts @allenporter
|
|
||
| @cached_property | ||
| def _is_shared(self) -> bool: | ||
| return any(d.duid == self._device_uid for d in self._home_data.received_devices) |
There was a problem hiding this comment.
We can check this up front in the constructor and if true, then set self._shared_device_uid then directly use that below when checking whether or not to check for shared device rooms
| def _room_name_map(self) -> dict[str, str]: | ||
| return {**self._home_data.rooms_name_map, **self._shared_room_names} | ||
|
|
||
| async def refresh(self) -> None: |
There was a problem hiding this comment.
The refresh_rooms method used to return all rooms such that we could overwrite the room list, but now it has a conditional to call a different API then we need to check that conditional again to correctly evaluate the room response type to know how to use it. I would say this could be improved to push the logic inside of refresh_rooms so that it always returns the final list of rooms.
| web_api_client.get_shared_device_rooms.assert_called_once_with(rooms_trait._device_uid) | ||
| web_api_client.get_rooms.assert_not_called() | ||
| finally: | ||
| rooms_trait._home_data.rooms = original_rooms |
There was a problem hiding this comment.
do we need this try/finally for this test?
| try: | ||
| # Mark the device as shared by adding it to received_devices | ||
| device = next(d for d in rooms_trait._home_data.devices if d.duid == rooms_trait._device_uid) | ||
| rooms_trait._home_data.received_devices = [device] |
There was a problem hiding this comment.
Instead of mutating internal state of the trait, add a new home data fixture input to device_fixture so that the test can build this up as input.
(Keep in mind there is other wiring like the product information etc that needs to be matched up)
| assert rooms_trait.rooms | ||
| assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=16, iot_id="2362048", raw_name="Example room 1") | ||
| assert rooms_trait.rooms[1] == NamedRoomMapping(segment_id=17, iot_id="9999999", raw_name="Office") | ||
| assert rooms_trait._home_data.rooms == original_rooms |
There was a problem hiding this comment.
don't think we need to assert on this
| assert rooms_trait.rooms | ||
| assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=16, iot_id="2362048", raw_name="Example room 1") | ||
| assert rooms_trait.rooms[1] == NamedRoomMapping(segment_id=17, iot_id="9999999", raw_name="Office") |
There was a problem hiding this comment.
Worth asserting that there are two?
| assert rooms_trait.rooms | |
| assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=16, iot_id="2362048", raw_name="Example room 1") | |
| assert rooms_trait.rooms[1] == NamedRoomMapping(segment_id=17, iot_id="9999999", raw_name="Office") | |
| assert rooms_trait.rooms == [ | |
| NamedRoomMapping(segment_id=16, iot_id="2362048", raw_name="Example room 1"), | |
| NamedRoomMapping(segment_id=17, iot_id="9999999", raw_name="Office"), | |
| ] |
| room_mapping_data = [[16, "2362048"], [17, "9999999"]] | ||
| mock_rpc_channel.send_command.side_effect = [room_mapping_data] | ||
|
|
||
| await rooms_trait.refresh() |
There was a problem hiding this comment.
worth asserting on the state before this is run? To verify these rooms weren't already set
|
will fix commit lint on merge |
relates to home-assistant/core#169639 (comment)
Add a new endpoint for getting shared room ids.