chore: address comments

Author: Lash-L

roborock/devices/traits/v1/home.py

@@ -138,7 +138,13 @@ async def _refresh_map_info(self, map_info) -> CombinedMapInfo:
                         # or if the room name isn't unknown.
                         rooms[room.segment_id] = room
 
-        await self._rooms_trait.resolve_unknown_room_names(rooms)
+        for segment_id, room in rooms.items():
+            if room.name == "Unknown":
+                rooms[segment_id] = NamedRoomMapping(
+                    segment_id=room.segment_id,
+                    iot_id=room.iot_id,
+                    name=f"Room {room.segment_id}",
+                )
 
         return CombinedMapInfo(
             map_flag=map_info.map_flag,

roborock/devices/traits/v1/rooms.py

@@ -33,13 +33,26 @@ class RoomsTrait(Rooms, common.V1TraitMixin):
 
     command = RoborockCommand.GET_ROOM_MAPPING
 
-    def __init__(self, home_data: HomeData, web_api: UserWebApiClient | None = None) -> None:
+    def __init__(self, home_data: HomeData, web_api: UserWebApiClient) -> None:
         """Initialize the RoomsTrait."""
         super().__init__()
         self._home_data = home_data
         self._web_api = web_api
         self._seen_unknown_room_iot_ids: set[str] = set()
 
+    async def refresh(self) -> None:
+        """Refresh room mappings and backfill unknown room names from the web API."""
+        response = await self.rpc_channel.send_command(self.command)
+        if not isinstance(response, list):
+            raise ValueError(f"Unexpected RoomsTrait response format: {response!r}")
+
+        segment_pairs = _extract_segment_pairs(response)
+        await self._populate_missing_home_data_rooms(segment_pairs)
+
+        new_data = self._parse_response(response)
+        self._update_trait_values(new_data)
+        _LOGGER.debug("Refreshed %s: %s", self.__class__.__name__, new_data)
+
     @property
     def _iot_id_room_name_map(self) -> dict[str, str]:
         """Returns a dictionary of Room IOT IDs to room names."""
@@ -73,30 +86,23 @@ def merge_home_data_rooms(self, rooms: list[HomeDataRoom]) -> None:
 
         self._home_data.rooms = updated_rooms
 
-    async def resolve_unknown_room_names(self, rooms: dict[int, NamedRoomMapping]) -> None:
-        """Resolve unknown room names using home data and web API fallbacks."""
-        unknown_room_iot_ids = {room.iot_id for room in rooms.values() if room.name == _DEFAULT_NAME}
+    async def _populate_missing_home_data_rooms(self, segment_pairs: list[tuple[int, str]]) -> None:
+        """Load missing room names into home data for newly-seen unknown room ids."""
+        name_map = self._iot_id_room_name_map
+        unknown_room_iot_ids = {
+            iot_id for _, iot_id in segment_pairs if name_map.get(iot_id, _DEFAULT_NAME) == _DEFAULT_NAME
+        }
         new_unknown_room_iot_ids = unknown_room_iot_ids - self._seen_unknown_room_iot_ids
-        web_room_names: dict[str, str] = {}
-
-        if self._web_api and new_unknown_room_iot_ids:
-            try:
-                web_rooms = await self._web_api.get_rooms()
-            except Exception:
-                _LOGGER.debug("Failed to fetch rooms from web API", exc_info=True)
-            else:
-                if web_rooms:
-                    web_room_names = {str(room.id): room.name for room in web_rooms}
-                    self.merge_home_data_rooms(web_rooms)
-
-        for segment_id, room in rooms.items():
-            if room.name != _DEFAULT_NAME:
-                continue
-            rooms[segment_id] = NamedRoomMapping(
-                segment_id=room.segment_id,
-                iot_id=room.iot_id,
-                name=web_room_names.get(room.iot_id, f"Room {room.segment_id}"),
-            )
+        if not new_unknown_room_iot_ids:
+            return
+
+        try:
+            web_rooms = await self._web_api.get_rooms()
+        except Exception:
+            _LOGGER.debug("Failed to fetch rooms from web API", exc_info=True)
+        else:
+            if web_rooms:
+                self.merge_home_data_rooms(web_rooms)
 
         self._seen_unknown_room_iot_ids.update(unknown_room_iot_ids)
 

tests/devices/traits/v1/test_home.py

@@ -140,11 +140,10 @@ def rooms_trait(device: RoborockDevice) -> RoomsTrait:
 
 
 @pytest.fixture
-def mock_web_api() -> AsyncMock:
-    """Create a mock web API client."""
-    mock = AsyncMock()
-    mock.get_rooms.return_value = []
-    return mock
+def mock_web_api(web_api_client: AsyncMock) -> AsyncMock:
+    """Alias the shared web API fixture for readability in this module."""
+    web_api_client.get_rooms.return_value = []
+    return web_api_client
 
 
 @pytest.fixture
@@ -154,10 +153,8 @@ def home_trait(
     map_content_trait: MapContentTrait,
     rooms_trait: RoomsTrait,
     device_cache: DeviceCache,
-    mock_web_api: AsyncMock,
 ) -> HomeTrait:
     """Create a HomeTrait instance with mocked dependencies."""
-    rooms_trait._web_api = mock_web_api
     return HomeTrait(status_trait, maps_trait, map_content_trait, rooms_trait, device_cache)
 
 
@@ -630,7 +627,6 @@ async def test_single_map_no_switching(
 async def test_refresh_map_info_room_override_and_addition_logic(
     home_trait: HomeTrait,
     rooms_trait: RoomsTrait,
-    mock_web_api: AsyncMock,
 ) -> None:
     """Test the room override and addition logic in _refresh_map_info.
 
@@ -671,9 +667,6 @@ async def test_refresh_map_info_room_override_and_addition_logic(
         NamedRoomMapping(segment_id=18, iot_id="2362041", name="Example room 3"),  # Not in map_info, should be added
     ]
 
-    # Mock web API to return empty rooms (no resolution)
-    mock_web_api.get_rooms.return_value = []
-
     # Mock rooms_trait.refresh to prevent actual device calls
     with patch.object(rooms_trait, "refresh", new_callable=AsyncMock):
         result = await home_trait._refresh_map_info(map_info)
@@ -708,32 +701,29 @@ async def test_refresh_map_info_room_override_and_addition_logic(
 
 async def test_get_rooms_resolves_unknown_rooms(
     home_trait: HomeTrait,
-    rooms_trait: RoomsTrait,
+    mock_rpc_channel: AsyncMock,
     mock_web_api: AsyncMock,
 ) -> None:
     """Test that get_rooms from web API resolves unknown room names."""
+    room_mapping_data = [[16, "9999801"], [17, "9999802"]]
+    mock_rpc_channel.send_command.side_effect = [room_mapping_data]
+
     map_info = MultiMapsListMapInfo(
         map_flag=0,
         name="Test Map",
         rooms=[
-            MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None),
-            MultiMapsListRoom(id=17, iot_name_id="2362044", iot_name=None),
+            MultiMapsListRoom(id=16, iot_name_id="9999801", iot_name=None),
+            MultiMapsListRoom(id=17, iot_name_id="9999802", iot_name=None),
         ],
     )
 
-    rooms_trait.rooms = [
-        NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-        NamedRoomMapping(segment_id=17, iot_id="2362044", name="Unknown"),
-    ]
-
     # Web API returns fresh room names
     mock_web_api.get_rooms.return_value = [
-        HomeDataRoom(id=2362048, name="Living Room"),
-        HomeDataRoom(id=2362044, name="Kitchen"),
+        HomeDataRoom(id=9999801, name="Living Room"),
+        HomeDataRoom(id=9999802, name="Kitchen"),
     ]
 
-    with patch.object(rooms_trait, "refresh", new_callable=AsyncMock):
-        result = await home_trait._refresh_map_info(map_info)
+    result = await home_trait._refresh_map_info(map_info)
 
     sorted_rooms = sorted(result.rooms, key=lambda r: r.segment_id)
     assert sorted_rooms[0].name == "Living Room"
@@ -743,28 +733,26 @@ async def test_get_rooms_resolves_unknown_rooms(
 
 async def test_get_rooms_called_once_for_same_unknown_room(
     home_trait: HomeTrait,
-    rooms_trait: RoomsTrait,
+    mock_rpc_channel: AsyncMock,
     mock_web_api: AsyncMock,
 ) -> None:
     """Test that get_rooms is not re-called for an already-seen unknown room."""
+    room_mapping_data = [[16, "9999701"]]
+    mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data]
+
     map_info = MultiMapsListMapInfo(
         map_flag=42,
         name="Test Map",
         rooms=[
-            MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None),
+            MultiMapsListRoom(id=16, iot_name_id="9999701", iot_name=None),
         ],
     )
 
-    rooms_trait.rooms = [
-        NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-    ]
-
     # Web API returns empty (no resolution)
     mock_web_api.get_rooms.return_value = []
 
-    with patch.object(rooms_trait, "refresh", new_callable=AsyncMock):
-        result1 = await home_trait._refresh_map_info(map_info)
-        result2 = await home_trait._refresh_map_info(map_info)
+    result1 = await home_trait._refresh_map_info(map_info)
+    result2 = await home_trait._refresh_map_info(map_info)
 
     # Both calls should fall back to "Room 16"
     assert result1.rooms[0].name == "Room 16"
@@ -775,34 +763,27 @@ async def test_get_rooms_called_once_for_same_unknown_room(
 
 async def test_get_rooms_called_again_for_new_unknown_room(
     home_trait: HomeTrait,
-    rooms_trait: RoomsTrait,
+    mock_rpc_channel: AsyncMock,
     mock_web_api: AsyncMock,
 ) -> None:
     """Test that get_rooms is called again when a new unknown room appears."""
+    room_mapping_data_1 = [[16, "9999601"]]
+    room_mapping_data_2 = [[16, "9999601"], [17, "9999602"]]
+    mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2]
+
     map_info = MultiMapsListMapInfo(
         map_flag=42,
         name="Test Map",
         rooms=[
-            MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None),
+            MultiMapsListRoom(id=16, iot_name_id="9999601", iot_name=None),
         ],
     )
 
-    rooms_trait.rooms = [
-        NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-    ]
-
     # Web API returns empty (no resolution)
     mock_web_api.get_rooms.return_value = []
 
-    with patch.object(rooms_trait, "refresh", new_callable=AsyncMock):
-        result1 = await home_trait._refresh_map_info(map_info)
-
-        # Add a brand-new unknown room for the same map
-        rooms_trait.rooms = [
-            NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-            NamedRoomMapping(segment_id=17, iot_id="2362044", name="Unknown"),
-        ]
-        result2 = await home_trait._refresh_map_info(map_info)
+    result1 = await home_trait._refresh_map_info(map_info)
+    result2 = await home_trait._refresh_map_info(map_info)
 
     assert sorted(room.name for room in result1.rooms) == ["Room 16"]
     assert sorted(room.name for room in result2.rooms) == ["Room 16", "Room 17"]
@@ -811,56 +792,48 @@ async def test_get_rooms_called_again_for_new_unknown_room(
 
 async def test_get_rooms_called_again_for_new_unknown_iot_id_same_segment(
     home_trait: HomeTrait,
-    rooms_trait: RoomsTrait,
+    mock_rpc_channel: AsyncMock,
     mock_web_api: AsyncMock,
 ) -> None:
     """Test that get_rooms is called again for a new unknown iot_id on the same segment."""
+    room_mapping_data_1 = [[16, "9999501"]]
+    room_mapping_data_2 = [[16, "9999502"]]
+    mock_rpc_channel.send_command.side_effect = [room_mapping_data_1, room_mapping_data_2]
+
     map_info = MultiMapsListMapInfo(
         map_flag=42,
         name="Test Map",
     )
 
-    rooms_trait.rooms = [
-        NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-    ]
-
     mock_web_api.get_rooms.return_value = []
 
-    with patch.object(rooms_trait, "refresh", new_callable=AsyncMock):
-        await home_trait._refresh_map_info(map_info)
-
-        # Same segment, but now with a different iot_id.
-        rooms_trait.rooms = [
-            NamedRoomMapping(segment_id=16, iot_id="2362999", name="Unknown"),
-        ]
-        await home_trait._refresh_map_info(map_info)
+    await home_trait._refresh_map_info(map_info)
+    await home_trait._refresh_map_info(map_info)
 
     assert mock_web_api.get_rooms.call_count == 2
 
 
 async def test_get_rooms_failure_falls_back_to_room_segment_id(
     home_trait: HomeTrait,
-    rooms_trait: RoomsTrait,
+    mock_rpc_channel: AsyncMock,
     mock_web_api: AsyncMock,
 ) -> None:
     """Test that get_rooms failure gracefully falls back to 'Room {segment_id}'."""
+    room_mapping_data = [[16, "9999401"]]
+    mock_rpc_channel.send_command.side_effect = [room_mapping_data]
+
     map_info = MultiMapsListMapInfo(
         map_flag=7,
         name="Test Map",
         rooms=[
-            MultiMapsListRoom(id=16, iot_name_id="2362048", iot_name=None),
+            MultiMapsListRoom(id=16, iot_name_id="9999401", iot_name=None),
         ],
     )
 
-    rooms_trait.rooms = [
-        NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-    ]
-
     # Web API raises an exception
     mock_web_api.get_rooms.side_effect = Exception("API error")
 
-    with patch.object(rooms_trait, "refresh", new_callable=AsyncMock):
-        result = await home_trait._refresh_map_info(map_info)
+    result = await home_trait._refresh_map_info(map_info)
 
     assert result.rooms[0].name == "Room 16"
     mock_web_api.get_rooms.assert_called_once()

tests/devices/traits/v1/test_rooms.py

@@ -87,40 +87,44 @@ def test_merge_home_data_rooms_appends_new_rooms(rooms_trait: RoomsTrait) -> Non
     assert home_data_rooms["9999999"] == "Office"
 
 
-async def test_resolve_unknown_room_names_web_api_called_once(
+async def test_refresh_unknown_room_names_web_api_called_once(
     rooms_trait: RoomsTrait,
     web_api_client: AsyncMock,
+    mock_rpc_channel: AsyncMock,
 ) -> None:
     """Test unknown room IDs trigger one web lookup per iot_id."""
     web_api_client.get_rooms.return_value = [
-        HomeDataRoom(id=2362048, name="Living Room"),
+        HomeDataRoom(id=9999911, name="Living Room"),
     ]
 
-    room_map = {
-        16: NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-    }
-    await rooms_trait.resolve_unknown_room_names(room_map)
-    assert room_map[16].name == "Living Room"
-
-    second_room_map = {
-        16: NamedRoomMapping(segment_id=16, iot_id="2362048", name="Unknown"),
-    }
-    await rooms_trait.resolve_unknown_room_names(second_room_map)
-    assert second_room_map[16].name == "Room 16"
+    room_mapping_data = [[16, "9999911"]]
+    mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data]
+
+    await rooms_trait.refresh()
+    assert rooms_trait.rooms
+    assert rooms_trait.rooms[0].name == "Living Room"
+
+    await rooms_trait.refresh()
+    assert rooms_trait.rooms
+    assert rooms_trait.rooms[0].name == "Living Room"
     web_api_client.get_rooms.assert_called_once()
 
 
-async def test_resolve_unknown_room_names_falls_back_to_segment_id(
+async def test_refresh_unknown_room_names_unresolved_keeps_unknown(
     rooms_trait: RoomsTrait,
     web_api_client: AsyncMock,
+    mock_rpc_channel: AsyncMock,
 ) -> None:
-    """Test unresolved unknown names use Room {segment_id} fallback."""
+    """Test unresolved unknown names stay unknown in RoomsTrait."""
     web_api_client.get_rooms.return_value = []
-    room_map = {
-        33: NamedRoomMapping(segment_id=33, iot_id="9999911", name="Unknown"),
-    }
+    room_mapping_data = [[33, "9999922"]]
+    mock_rpc_channel.send_command.side_effect = [room_mapping_data, room_mapping_data]
 
-    await rooms_trait.resolve_unknown_room_names(room_map)
+    await rooms_trait.refresh()
+    assert rooms_trait.rooms
+    assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Unknown")
 
-    assert room_map[33].name == "Room 33"
+    await rooms_trait.refresh()
+    assert rooms_trait.rooms
+    assert rooms_trait.rooms[0] == NamedRoomMapping(segment_id=33, iot_id="9999922", name="Unknown")
     web_api_client.get_rooms.assert_called_once()