perf: dedupe lovelace/dashboards/list in ha_config_set_dashboard (#1085)

src/ha_mcp/tools/tools_config_dashboards.py

@@ -265,22 +265,37 @@ def _should_lazy_resolve(error_msg: str) -> bool:
     return _LAZY_RESOLVE_TRIGGER in error_msg
 
 
-async def _resolve_dashboard(client: Any, identifier: str) -> dict[str, str] | None:
+async def _resolve_dashboard(
+    client: Any, identifier: str
+) -> tuple[dict[str, str] | None, list[dict[str, Any]] | None]:
     """Resolve a dashboard identifier (url_path or internal id) to both forms.
 
-    Calls ``lovelace/dashboards/list`` and returns
-    ``{"url_path": ..., "id": ...}`` when the identifier matches either field
-    on a registry entry that has both fields populated; otherwise returns
-    ``None``. Always pays the round-trip when called.
+    Calls ``lovelace/dashboards/list`` and returns a 2-tuple
+    ``(match, dashboards)``:
 
-    Two call sites:
+    - ``match`` is ``{"url_path": ..., "id": ...}`` when the identifier
+      matches either field on a registry entry that has both fields
+      populated; otherwise ``None``.
+    - ``dashboards`` is the raw list as returned by HA when the
+      response shape is recognised (dict-with-``result`` or bare list);
+      ``None`` when the shape was unexpected and a warning was logged.
+
+    Returning ``dashboards`` alongside ``match`` lets callers reuse the
+    list for follow-on checks (existence, id lookup) instead of paying
+    a second ``lovelace/dashboards/list`` round-trip — see issue #1085.
+
+    Three call sites:
     - **Lazy fallback** (``_lazy_resolve_and_retry``): only invoked after
       ``lovelace/config`` rejected the identifier with
       ``_LAZY_RESOLVE_TRIGGER`` — the round-trip is gated by the caller.
+      Discards ``dashboards``.
     - **Eager pre-resolve** (``ha_config_set_dashboard``): invoked before
       hyphen validation so callers may pass either form; gated on a
       cheap heuristic ("no hyphen, not 'lovelace'") rather than an error
-      from HA.
+      from HA. Reuses ``dashboards`` for the existence-check below.
+    - **Delete** (``ha_config_delete_dashboard``): resolves either form
+      to the registry id before issuing the delete. Discards
+      ``dashboards``.
     """
     result = await client.send_websocket_message({"type": "lovelace/dashboards/list"})
     if isinstance(result, dict) and "result" in result:
@@ -297,7 +312,7 @@ async def _resolve_dashboard(client: Any, identifier: str) -> dict[str, str] | N
             "treating as no-match",
             type(result).__name__,
         )
-        return None
+        return None, None
 
     for d in dashboards:
         if d.get("id") == identifier or d.get("url_path") == identifier:
@@ -309,8 +324,8 @@ async def _resolve_dashboard(client: Any, identifier: str) -> dict[str, str] | N
                 # would be silently used by callers (e.g.
                 # ``delete_dashboard`` would forward ``resolved_id=""``).
                 continue
-            return {"url_path": url_path, "id": entry_id}
-    return None
+            return {"url_path": url_path, "id": entry_id}, dashboards
+    return None, dashboards
 
 
 @overload
@@ -376,7 +391,9 @@ async def _lazy_resolve_and_retry(
         return url_path, response
 
     try:
-        resolved = await _resolve_dashboard(client, url_path)
+        # Lazy fallback only needs the resolution; the dashboards list
+        # is not reused on this path.
+        resolved, _ = await _resolve_dashboard(client, url_path)
     except Exception as resolver_exc:
         # Resolver itself raised (timeout, network blip, etc.). Don't let
         # this exception escape and replace the original HA error with
@@ -941,12 +958,18 @@ async def ha_config_set_dashboard(
             # ``resolved_from`` on the success response so callers can
             # detect this redirect.
             pre_resolved_from: str | None = None
+            # When the pre-resolver fires and finds a match, ``_resolve_dashboard``
+            # has already fetched ``lovelace/dashboards/list``. Capture that list
+            # so the existence-check site below can reuse it instead of paying
+            # a second round-trip (issue #1085).
+            pre_fetched_dashboards: list[dict[str, Any]] | None = None
             if "-" not in url_path and url_path != "lovelace":
-                resolved = await _resolve_dashboard(client, url_path)
+                resolved, dashboards = await _resolve_dashboard(client, url_path)
                 if resolved is not None and resolved["url_path"]:
                     original_url_path = url_path
                     url_path = resolved["url_path"]
                     pre_resolved_from = original_url_path
+                    pre_fetched_dashboards = dashboards
                     logger.info(
                         "ha_config_set_dashboard pre-resolver mapped %r -> %r",
                         original_url_path,
@@ -1129,16 +1152,23 @@ async def ha_config_set_dashboard(
                     transform_result["resolved_from"] = pre_resolved_from
                 return transform_result
 
-            # Check if dashboard exists
-            result = await client.send_websocket_message(
-                {"type": "lovelace/dashboards/list"}
-            )
-            if isinstance(result, dict) and "result" in result:
-                existing_dashboards = result["result"]
-            elif isinstance(result, list):
-                existing_dashboards = result
+            # Check if dashboard exists. When the pre-resolver fired
+            # and matched (internal-id branch), reuse its already-fetched
+            # ``lovelace/dashboards/list`` response to skip a redundant
+            # round-trip — the matched dashboard is guaranteed present in
+            # that list (issue #1085).
+            if pre_fetched_dashboards is not None:
+                existing_dashboards = pre_fetched_dashboards
             else:
-                existing_dashboards = []
+                result = await client.send_websocket_message(
+                    {"type": "lovelace/dashboards/list"}
+                )
+                if isinstance(result, dict) and "result" in result:
+                    existing_dashboards = result["result"]
+                elif isinstance(result, list):
+                    existing_dashboards = result
+                else:
+                    existing_dashboards = []
             dashboard_exists = any(
                 d.get("url_path") == url_path for d in existing_dashboards
             )
@@ -1407,7 +1437,9 @@ async def ha_config_delete_dashboard(
         Note: The default dashboard cannot be deleted via this method.
         """
         try:
-            resolved = await _resolve_dashboard(client, url_path)
+            # Delete-path doesn't need the dashboards list — only the
+            # resolved id is forwarded to lovelace/dashboards/delete.
+            resolved, _ = await _resolve_dashboard(client, url_path)
             if resolved is None:
                 raise_tool_error(
                     create_resource_not_found_error(

tests/src/unit/test_set_dashboard_metadata.py

@@ -153,3 +153,115 @@ async def test_false_booleans_are_not_filtered_out(self, set_tool, mock_client):
         meta_call = mock_client.send_websocket_message.call_args_list[1][0][0]
         assert meta_call["require_admin"] is False
         assert meta_call["show_in_sidebar"] is False
+
+
+class TestSetDashboardListCallDedup:
+    """Issue #1085: when the pre-resolver fires (internal-id branch),
+    the existence-check site reuses the pre-fetched dashboards list
+    rather than issuing a second ``lovelace/dashboards/list`` round-trip.
+
+    The other-branch tests act as regression guards so a future change
+    that re-introduces a redundant list call (or accidentally drops the
+    one fetch on the canonical-url_path branch) is caught here."""
+
+    @pytest.fixture
+    def mock_mcp(self):
+        mcp = MagicMock()
+        self.registered_tools: dict = {}
+
+        def tool_decorator(*args, **kwargs):
+            def wrapper(func):
+                self.registered_tools[func.__name__] = func
+                return func
+
+            return wrapper
+
+        mcp.tool = tool_decorator
+        return mcp
+
+    @pytest.fixture
+    def mock_client(self):
+        client = MagicMock()
+        client.send_websocket_message = AsyncMock()
+        return client
+
+    @pytest.fixture
+    def set_tool(self, mock_mcp, mock_client):
+        from ha_mcp.tools.tools_config_dashboards import register_config_dashboard_tools
+
+        register_config_dashboard_tools(mock_mcp, mock_client)
+        return self.registered_tools["ha_config_set_dashboard"]
+
+    @staticmethod
+    def _list_call_count(mock_client) -> int:
+        return sum(
+            1
+            for c in mock_client.send_websocket_message.call_args_list
+            if c.args and c.args[0].get("type") == "lovelace/dashboards/list"
+        )
+
+    @pytest.mark.asyncio
+    async def test_internal_id_branch_calls_list_only_once(
+        self, set_tool, mock_client
+    ):
+        """Pre-resolver fires (hyphenless ``my_dash``) and matches; the
+        existence-check site MUST reuse that list instead of fetching
+        again. Total ``lovelace/dashboards/list`` calls = 1."""
+        dashboards_list = {"result": [{"url_path": "my-dash", "id": "my_dash"}]}
+        mock_client.send_websocket_message.side_effect = [
+            dashboards_list,  # pre-resolver fetch
+            {"success": True},  # metadata update
+        ]
+
+        result = await set_tool(url_path="my_dash", title="Renamed")
+
+        assert self._list_call_count(mock_client) == 1, (
+            "internal-id branch must reuse the pre-resolver's dashboards "
+            "list — second lovelace/dashboards/list round-trip is the "
+            "regression issue #1085 closes"
+        )
+        assert result["success"] is True
+        # Pre-resolver rewrote my_dash -> my-dash; surface marker stays.
+        assert result.get("resolved_from") == "my_dash"
+        # Metadata update did fire on the canonical url_path.
+        meta_call = mock_client.send_websocket_message.call_args_list[1].args[0]
+        assert meta_call["type"] == "lovelace/dashboards/update"
+        assert meta_call["dashboard_id"] == "my_dash"
+
+    @pytest.mark.asyncio
+    async def test_canonical_url_path_branch_still_calls_list_once(
+        self, set_tool, mock_client
+    ):
+        """Already-canonical ``my-dash`` (hyphen present) skips the
+        pre-resolver; the existence-check site still fetches once.
+        Regression guard: total list calls = 1, not 0 (pre-resolver
+        didn't fire) and not 2 (no redundant fetch)."""
+        mock_client.send_websocket_message.side_effect = [
+            {"result": [{"url_path": "my-dash", "id": "my_dash"}]},
+            {"success": True},
+        ]
+
+        result = await set_tool(url_path="my-dash", title="Renamed")
+
+        assert self._list_call_count(mock_client) == 1
+        assert result["success"] is True
+        assert result.get("resolved_from") is None
+
+    @pytest.mark.asyncio
+    async def test_internal_id_no_match_falls_through_to_hyphen_check(
+        self, set_tool, mock_client
+    ):
+        """Hyphenless identifier with no matching dashboard: pre-resolver
+        fetches the list, finds no match, ``url_path`` stays
+        unchanged, then fails the hyphen-validation check before any
+        existence-check fetch can fire. Total list calls = 1."""
+        mock_client.send_websocket_message.side_effect = [
+            {"result": [{"url_path": "other-dash", "id": "other_dash"}]},
+        ]
+
+        with pytest.raises(ToolError) as exc_info:
+            await set_tool(url_path="ghost", title="X")
+
+        body = json.loads(str(exc_info.value))
+        assert "url_path must contain a hyphen" in body["error"]["message"]
+        assert self._list_call_count(mock_client) == 1

tests/src/unit/test_tools_config_dashboards.py

@@ -89,68 +89,84 @@ def test_legitimate_empty_dashboard_message_does_not_match(self):
 
 
 class TestResolveDashboard:
+    """``_resolve_dashboard`` returns ``(match, dashboards)``: the match
+    (or ``None``) plus the raw dashboards list (or ``None`` on
+    unexpected shape). The list is what enables list-call dedup at the
+    pre-resolver call site (issue #1085); test both arms here."""
+
     async def test_match_by_url_path(self, fake_client):
-        fake_client.send_websocket_message.return_value = {
-            "result": [
-                {"url_path": "my-dash", "id": "my_dash"},
-                {"url_path": "other", "id": "other_id"},
-            ]
-        }
-        result = await _resolve_dashboard(fake_client, "my-dash")
-        assert result == {"url_path": "my-dash", "id": "my_dash"}
+        dashboards_list = [
+            {"url_path": "my-dash", "id": "my_dash"},
+            {"url_path": "other", "id": "other_id"},
+        ]
+        fake_client.send_websocket_message.return_value = {"result": dashboards_list}
+        match, dashboards = await _resolve_dashboard(fake_client, "my-dash")
+        assert match == {"url_path": "my-dash", "id": "my_dash"}
+        assert dashboards == dashboards_list
 
     async def test_match_by_internal_id(self, fake_client):
-        fake_client.send_websocket_message.return_value = {
-            "result": [{"url_path": "my-dash", "id": "my_dash"}]
-        }
-        result = await _resolve_dashboard(fake_client, "my_dash")
-        assert result == {"url_path": "my-dash", "id": "my_dash"}
+        dashboards_list = [{"url_path": "my-dash", "id": "my_dash"}]
+        fake_client.send_websocket_message.return_value = {"result": dashboards_list}
+        match, dashboards = await _resolve_dashboard(fake_client, "my_dash")
+        assert match == {"url_path": "my-dash", "id": "my_dash"}
+        assert dashboards == dashboards_list
 
     async def test_response_as_bare_list(self, fake_client):
         # Older HA versions / different response shapes return the list
         # directly rather than wrapped in {"result": ...}.
-        fake_client.send_websocket_message.return_value = [
-            {"url_path": "my-dash", "id": "my_dash"}
-        ]
-        result = await _resolve_dashboard(fake_client, "my_dash")
-        assert result == {"url_path": "my-dash", "id": "my_dash"}
-
-    async def test_no_match_returns_none(self, fake_client):
-        fake_client.send_websocket_message.return_value = {
-            "result": [{"url_path": "my-dash", "id": "my_dash"}]
-        }
-        assert await _resolve_dashboard(fake_client, "nonexistent") is None
-
-    async def test_malformed_shape_logs_warning_and_returns_none(
+        dashboards_list = [{"url_path": "my-dash", "id": "my_dash"}]
+        fake_client.send_websocket_message.return_value = dashboards_list
+        match, dashboards = await _resolve_dashboard(fake_client, "my_dash")
+        assert match == {"url_path": "my-dash", "id": "my_dash"}
+        assert dashboards == dashboards_list
+
+    async def test_no_match_still_returns_dashboards_list(self, fake_client):
+        # When the identifier doesn't match any dashboard, ``match`` is
+        # None but ``dashboards`` still carries the fetched list — the
+        # fetch happened, only the match check failed.
+        dashboards_list = [{"url_path": "my-dash", "id": "my_dash"}]
+        fake_client.send_websocket_message.return_value = {"result": dashboards_list}
+        match, dashboards = await _resolve_dashboard(fake_client, "nonexistent")
+        assert match is None
+        assert dashboards == dashboards_list
+
+    async def test_malformed_shape_logs_warning_and_returns_none_pair(
         self, fake_client, caplog
     ):
         # Neither dict-with-result nor list — could be a future HA shape
         # change or an error envelope. Must surface as a logger.warning,
-        # not silently degrade to "always no match".
+        # not silently degrade to "always no match". Both elements of
+        # the tuple are None so callers know the fetch failed and they
+        # can fall back to a fresh fetch instead of treating ``[]`` as
+        # an authoritative empty registry.
         fake_client.send_websocket_message.return_value = "unexpected string"
         with caplog.at_level(
             logging.WARNING, logger="ha_mcp.tools.tools_config_dashboards"
         ):
-            result = await _resolve_dashboard(fake_client, "anything")
-        assert result is None
+            match, dashboards = await _resolve_dashboard(fake_client, "anything")
+        assert match is None
+        assert dashboards is None
         assert any("unexpected shape" in rec.message for rec in caplog.records), (
             f"expected an 'unexpected shape' warning; got {caplog.records}"
         )
 
-    async def test_missing_url_path_in_match_returns_none(self, fake_client):
+    async def test_missing_url_path_in_match_returns_none_match(self, fake_client):
         # Malformed registry entry where the matching dashboard is
-        # missing one of the required fields. Must skip / return None
-        # rather than forwarding empty strings to delete_dashboard.
-        fake_client.send_websocket_message.return_value = {
-            "result": [{"id": "my_dash"}]  # url_path missing entirely
-        }
-        assert await _resolve_dashboard(fake_client, "my_dash") is None
-
-    async def test_empty_id_in_match_returns_none(self, fake_client):
-        fake_client.send_websocket_message.return_value = {
-            "result": [{"url_path": "my-dash", "id": ""}]
-        }
-        assert await _resolve_dashboard(fake_client, "my-dash") is None
+        # missing one of the required fields. Match is None (skipped
+        # rather than forwarding empty strings to delete_dashboard) but
+        # the list itself is still returned.
+        dashboards_list = [{"id": "my_dash"}]  # url_path missing entirely
+        fake_client.send_websocket_message.return_value = {"result": dashboards_list}
+        match, dashboards = await _resolve_dashboard(fake_client, "my_dash")
+        assert match is None
+        assert dashboards == dashboards_list
+
+    async def test_empty_id_in_match_returns_none_match(self, fake_client):
+        dashboards_list = [{"url_path": "my-dash", "id": ""}]
+        fake_client.send_websocket_message.return_value = {"result": dashboards_list}
+        match, dashboards = await _resolve_dashboard(fake_client, "my-dash")
+        assert match is None
+        assert dashboards == dashboards_list
 
 
 # -----------------------------------------------------------------------------