This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new e041f253856 fix(mcp): return error when target_tab not found in
add_chart_to_existing_dashboard (#40124)
e041f253856 is described below
commit e041f2538568ae147eade505a46f5c82d4a06cf1
Author: Amin Ghadersohi <[email protected]>
AuthorDate: Wed May 27 11:29:43 2026 -0700
fix(mcp): return error when target_tab not found in
add_chart_to_existing_dashboard (#40124)
---
superset/mcp_service/dashboard/schemas.py | 25 ++-
.../tool/add_chart_to_existing_dashboard.py | 91 +++++++++--
.../tool/test_add_chart_to_existing_dashboard.py | 52 ++++++
.../dashboard/tool/test_dashboard_generation.py | 174 +++++++++++++++++++--
4 files changed, 319 insertions(+), 23 deletions(-)
diff --git a/superset/mcp_service/dashboard/schemas.py
b/superset/mcp_service/dashboard/schemas.py
index 9b889ff27ee..8904e9e0811 100644
--- a/superset/mcp_service/dashboard/schemas.py
+++ b/superset/mcp_service/dashboard/schemas.py
@@ -487,7 +487,17 @@ class AddChartToDashboardRequest(BaseModel):
)
chart_id: int = Field(..., description="ID of the chart to add to the
dashboard")
target_tab: str | None = Field(
- None, description="Target tab name (if dashboard has tabs)"
+ None,
+ min_length=1,
+ description=(
+ "Tab to place the chart in. Accepts a tab display name "
+ "(e.g. 'Sales') or a tab component ID (e.g. 'TAB-abc123'). "
+ "Display-name matching is case-insensitive and strips all emoji; "
+ "component ID matching is case-sensitive and exact. "
+ "When not found, the error response lists all available tab names.
"
+ "When omitted on a tabbed dashboard the chart is placed in the "
+ "first tab."
+ ),
)
@@ -514,6 +524,19 @@ class AddChartToDashboardResponse(BaseModel):
),
)
+ @field_validator("error")
+ @classmethod
+ def sanitize_error_for_llm_context(cls, value: str | None) -> str | None:
+ """Wrap error text before it is exposed to LLM context.
+
+ The error may echo user-supplied target_tab or dashboard-controlled tab
+ labels — both must be wrapped so the LLM treats them as data, not
+ instructions.
+ """
+ if value is None:
+ return value
+ return sanitize_for_llm_context(value, field_path=("error",))
+
class GenerateDashboardRequest(BaseModel):
"""Request schema for generating a dashboard."""
diff --git
a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
index 72fcd482a55..4f64d53cbd0 100644
--- a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
@@ -149,29 +149,50 @@ def _first_tab_from_groups(
return None
+def _collect_available_tab_names(layout: Dict[str, Any]) -> list[str]:
+ """Collect display entries (label + component ID) for all TAB components.
+
+ Always includes the component ID so callers can retry unambiguously even
+ when multiple tabs share the same display name or a label is blank.
+ """
+ entries: list[str] = []
+ for tabs_children in _collect_tabs_groups(layout):
+ for tab_id in tabs_children:
+ tab = layout.get(tab_id)
+ if not tab or tab.get("type") != "TAB":
+ continue
+ text = (tab.get("meta") or {}).get("text", "")
+ entries.append(f"{text} ({tab_id})" if text else tab_id)
+ return entries
+
+
def _find_tab_insert_target(
layout: Dict[str, Any], target_tab: str | None = None
) -> str | None:
"""
Detect if the dashboard uses tabs and return the appropriate tab's ID.
- If *target_tab* is provided the function first tries to match it against
- tab ``meta.text`` (display name) or the raw component ID. When no match
- is found (or *target_tab* is ``None``) the first ``TAB`` child is used as
- a fallback so that new rows are still placed inside the tab structure
- rather than directly under ``GRID_ID``.
+ When *target_tab* is ``None`` the function returns the first TAB child so
+ that new rows are placed inside the tab structure rather than directly
+ under ``GRID_ID``.
+
+ When *target_tab* is provided the function tries to match it against tab
+ ``meta.text`` (display name) or the raw component ID. If no match is
+ found ``None`` is returned — the caller is responsible for surfacing an
+ error rather than silently placing the chart in the wrong tab.
Returns:
- The ID of the matched (or first) TAB component, or ``None`` if the
- dashboard does not use top-level tabs.
+ The ID of the matched (or first) TAB component, or ``None``.
"""
groups = _collect_tabs_groups(layout)
- if target_tab:
+ if target_tab is not None:
for tabs_children in groups:
matched = _match_tab_in_children(layout, tabs_children, target_tab)
if matched:
return matched
+ # target_tab specified but not found — signal mismatch to the caller.
+ return None
return _first_tab_from_groups(layout, groups)
@@ -316,6 +337,45 @@ def _ensure_layout_structure(
layout["DASHBOARD_VERSION_KEY"] = "v2"
+def _resolve_parent_container(
+ layout: Dict[str, Any],
+ dashboard_id: int,
+ target_tab: str | None,
+) -> tuple[str, None] | tuple[None, AddChartToDashboardResponse]:
+ """Return (parent_id, None) on success or (None, error_response) on
mismatch.
+
+ When *target_tab* is specified and not found the caller receives a
+ descriptive error listing available tabs rather than a silent fallback.
+ """
+ tab_target = _find_tab_insert_target(layout, target_tab=target_tab)
+
+ if target_tab is not None and tab_target is None:
+ available = _collect_available_tab_names(layout)
+ if available:
+ tab_list = ", ".join(available)
+ return None, AddChartToDashboardResponse(
+ dashboard=None,
+ dashboard_url=None,
+ position=None,
+ error=(
+ f"Tab '{target_tab}' not found in dashboard
{dashboard_id}. "
+ f"Available tabs: {tab_list}."
+ ),
+ )
+ return None, AddChartToDashboardResponse(
+ dashboard=None,
+ dashboard_url=None,
+ position=None,
+ error=(
+ f"Dashboard {dashboard_id} has no tabs. "
+ "Remove the target_tab parameter to add the chart to "
+ "the default grid layout."
+ ),
+ )
+
+ return (tab_target if tab_target else "GRID_ID", None)
+
+
def _find_and_authorize_dashboard(
dashboard_id: int,
) -> tuple[Any, AddChartToDashboardResponse | None]:
@@ -369,7 +429,7 @@ def _find_and_authorize_dashboard(
destructiveHint=False,
),
)
-def add_chart_to_existing_dashboard(
+def add_chart_to_existing_dashboard( # noqa: C901 — complexity is structural
(layout traversal + multi-step authorization), not accidental
request: AddChartToDashboardRequest, ctx: Context
) -> AddChartToDashboardResponse:
"""
@@ -443,11 +503,16 @@ def add_chart_to_existing_dashboard(
# Generate a unique ROW ID for the new row
row_key = _find_next_row_position(current_layout)
- # Detect tabbed dashboards and resolve target_tab by name or ID
- tab_target = _find_tab_insert_target(
- current_layout, target_tab=request.target_tab
+ # Detect tabbed dashboards and resolve target_tab by name or ID.
+ parent_id, tab_error = _resolve_parent_container(
+ current_layout, request.dashboard_id, request.target_tab
)
- parent_id = tab_target if tab_target else "GRID_ID"
+ if tab_error is not None:
+ return tab_error
+ if parent_id is None:
+ raise RuntimeError(
+ "unreachable: tab_error is None implies parent_id is str"
+ )
# Add chart, column, and row to layout
chart_key, column_key, row_key = _add_chart_to_layout(
diff --git
a/tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py
b/tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py
index de8058c92de..10cfa8045f1 100644
---
a/tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py
+++
b/tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py
@@ -295,3 +295,55 @@ async def test_successful_add(
assert "/superset/dashboard/1/" in content["dashboard_url"]
assert content["position"] is not None
assert "chart_key" in content["position"]
+
+
+def test_empty_target_tab_rejected_by_schema() -> None:
+ """Empty string target_tab is rejected at schema layer, not as 'Tab not
found'."""
+ from pydantic import ValidationError
+
+ from superset.mcp_service.dashboard.schemas import
AddChartToDashboardRequest
+
+ with pytest.raises(ValidationError):
+ AddChartToDashboardRequest(dashboard_id=1, chart_id=10, target_tab="")
+
+ # None is valid (tab omitted)
+ req = AddChartToDashboardRequest(dashboard_id=1, chart_id=10,
target_tab=None)
+ assert req.target_tab is None
+
+
+def test_add_chart_response_error_is_sanitized_for_llm_context() -> None:
+ """Error field wraps user-supplied target_tab and dashboard tab labels.
+
+ The error string echoes user-provided input (target_tab) and
+ dashboard-controlled tab labels. Both must be wrapped in
+ UNTRUSTED-CONTENT delimiters so the LLM treats them as data, not
+ instructions.
+ """
+ from superset.mcp_service.dashboard.schemas import
AddChartToDashboardResponse
+ from superset.mcp_service.utils.sanitization import (
+ LLM_CONTEXT_CLOSE_DELIMITER,
+ LLM_CONTEXT_OPEN_DELIMITER,
+ )
+
+ raw_error = (
+ "Tab 'malicious tab <script>alert(1)</script>' not found in dashboard
42. "
+ "Available tabs: Sales (TAB-abc), <b>Marketing</b> (TAB-xyz)."
+ )
+ response = AddChartToDashboardResponse(
+ dashboard=None,
+ dashboard_url=None,
+ position=None,
+ error=raw_error,
+ )
+
+ assert response.error is not None
+ assert LLM_CONTEXT_OPEN_DELIMITER in response.error
+ assert LLM_CONTEXT_CLOSE_DELIMITER in response.error
+ # Core text is still present inside the wrapper
+ assert "not found" in response.error
+ assert "Available tabs" in response.error
+ # None error is passed through unchanged
+ empty_response = AddChartToDashboardResponse(
+ dashboard=None, dashboard_url=None, position=None, error=None
+ )
+ assert empty_response.error is None
diff --git
a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
index b18d6a125bf..c7a07b724c7 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
@@ -32,6 +32,7 @@ from superset.mcp_service.chart.chart_utils import
DatasetValidationResult
from superset.mcp_service.dashboard.constants import generate_id
from superset.mcp_service.dashboard.tool.add_chart_to_existing_dashboard
import (
_add_chart_to_layout,
+ _collect_available_tab_names,
_ensure_layout_structure,
_find_next_row_position,
_find_tab_insert_target,
@@ -1059,6 +1060,112 @@ class TestAddChartToExistingDashboard:
assert "TAB-tab2" in chart_parents
assert "TAB-tab1" not in chart_parents
+ @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_add_chart_target_tab_not_found(
+ self, mock_db_session, mock_find_dashboard, mock_update_command,
mcp_server
+ ) -> None:
+ """target_tab specified but no matching tab → descriptive error listing
+ available tabs, not a silent fallback to the first tab."""
+ mock_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard")
+ mock_dashboard.slices = [_mock_chart(id=10)]
+ mock_dashboard.position_json = json.dumps(
+ {
+ "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type":
"ROOT"},
+ "GRID_ID": {
+ "children": ["TABS-abc123"],
+ "id": "GRID_ID",
+ "parents": ["ROOT_ID"],
+ "type": "GRID",
+ },
+ "TABS-abc123": {
+ "children": ["TAB-tab1", "TAB-tab2"],
+ "id": "TABS-abc123",
+ "parents": ["ROOT_ID", "GRID_ID"],
+ "type": "TABS",
+ },
+ "TAB-tab1": {
+ "children": [],
+ "id": "TAB-tab1",
+ "meta": {"text": "Overview"},
+ "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
+ "type": "TAB",
+ },
+ "TAB-tab2": {
+ "children": [],
+ "id": "TAB-tab2",
+ "meta": {"text": "Details"},
+ "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
+ "type": "TAB",
+ },
+ "DASHBOARD_VERSION_KEY": "v2",
+ }
+ )
+ mock_chart = _mock_chart(id=30)
+ mock_db_session.get.return_value = mock_chart
+ mock_find_dashboard.return_value = mock_dashboard
+
+ request = {"dashboard_id": 3, "chart_id": 30, "target_tab":
"Nonexistent Tab"}
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "add_chart_to_existing_dashboard", {"request": request}
+ )
+
+ assert result.structured_content["error"] is not None
+ error = result.structured_content["error"]
+ assert "Nonexistent Tab" in error
+ assert "not found" in error
+ # Available tabs listed with both label and component ID
+ assert "Overview" in error
+ assert "Details" in error
+ assert "TAB-tab1" in error
+ assert "TAB-tab2" in error
+ # No layout mutation should have been persisted
+ mock_update_command.assert_not_called()
+
+ @patch("superset.commands.dashboard.update.UpdateDashboardCommand")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_add_chart_target_tab_on_non_tabbed_dashboard(
+ self, mock_db_session, mock_find_dashboard, mock_update_command,
mcp_server
+ ) -> None:
+ """target_tab on a dashboard with no tabs → descriptive error."""
+ mock_dashboard = _mock_dashboard(id=5, title="Flat Dashboard")
+ mock_dashboard.slices = []
+ mock_dashboard.position_json = json.dumps(
+ {
+ "ROOT_ID": {"children": ["GRID_ID"], "id": "ROOT_ID", "type":
"ROOT"},
+ "GRID_ID": {
+ "children": [],
+ "id": "GRID_ID",
+ "parents": ["ROOT_ID"],
+ "type": "GRID",
+ },
+ "DASHBOARD_VERSION_KEY": "v2",
+ }
+ )
+ mock_chart = _mock_chart(id=99)
+ mock_db_session.get.return_value = mock_chart
+ mock_find_dashboard.return_value = mock_dashboard
+
+ request = {"dashboard_id": 5, "chart_id": 99, "target_tab": "Sales"}
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "add_chart_to_existing_dashboard", {"request": request}
+ )
+
+ assert result.structured_content["error"] is not None
+ error = result.structured_content["error"]
+ assert "no tabs" in error.lower()
+ assert "target_tab" in error
+ # No layout mutation should have been persisted
+ mock_update_command.assert_not_called()
+
@patch("superset.commands.dashboard.update.UpdateDashboardCommand")
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@patch("superset.db.session")
@@ -1311,9 +1418,9 @@ class TestLayoutHelpers:
}
assert _find_tab_insert_target(layout, target_tab="TAB-second") ==
"TAB-second"
- def test_find_tab_insert_target_unmatched_falls_back_to_first(self):
- """Test _find_tab_insert_target falls back to first tab when target_tab
- doesn't match any tab name or ID."""
+ def test_find_tab_insert_target_unmatched_returns_none(self):
+ """Test _find_tab_insert_target returns None when target_tab doesn't
+ match any tab name or ID, so the caller can return a descriptive
error."""
layout = {
"GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
"TABS-main": {"children": ["TAB-first", "TAB-second"], "type":
"TABS"},
@@ -1328,11 +1435,19 @@ class TestLayoutHelpers:
"meta": {"text": "Tab 2"},
},
}
- assert (
- _find_tab_insert_target(layout, target_tab="Nonexistent Tab") ==
"TAB-first"
- )
+ assert _find_tab_insert_target(layout, target_tab="Nonexistent Tab")
is None
+
+ def test_find_tab_insert_target_empty_string_returns_none(self) -> None:
+ """An empty-string target_tab is treated as specified-but-not-found,
+ not as 'no tab requested', so it returns None rather than first tab."""
+ layout = {
+ "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
+ "TABS-main": {"children": ["TAB-first"], "type": "TABS"},
+ "TAB-first": {"children": [], "type": "TAB", "meta": {"text": "Tab
1"}},
+ }
+ assert _find_tab_insert_target(layout, target_tab="") is None
- def test_find_tab_insert_target_tabs_under_root(self):
+ def test_find_tab_insert_target_tabs_under_root(self) -> None:
"""Test _find_tab_insert_target when TABS are under ROOT_ID (real
layout)."""
layout = {
"ROOT_ID": {"children": ["TABS-xxx"], "type": "ROOT"},
@@ -1343,7 +1458,7 @@ class TestLayoutHelpers:
}
assert _find_tab_insert_target(layout) == "TAB-a"
- def test_find_tab_insert_target_tabs_under_root_by_name(self):
+ def test_find_tab_insert_target_tabs_under_root_by_name(self) -> None:
"""Test _find_tab_insert_target matches tab name when TABS under
ROOT_ID."""
layout = {
"ROOT_ID": {"children": ["TABS-xxx"], "type": "ROOT"},
@@ -1354,10 +1469,51 @@ class TestLayoutHelpers:
}
assert _find_tab_insert_target(layout, target_tab="Details") == "TAB-b"
- def test_find_tab_insert_target_no_grid(self):
+ def test_find_tab_insert_target_no_grid(self) -> None:
"""Test _find_tab_insert_target with missing GRID_ID."""
assert _find_tab_insert_target({"ROOT_ID": {"type": "ROOT"}}) is None
+ def test_collect_available_tab_names_returns_display_names(self) -> None:
+ """_collect_available_tab_names returns label + component ID for each
tab."""
+ layout = {
+ "GRID_ID": {"children": ["TABS-x"], "type": "GRID"},
+ "TABS-x": {"children": ["TAB-a", "TAB-b"], "type": "TABS"},
+ "TAB-a": {"children": [], "type": "TAB", "meta": {"text":
"Overview"}},
+ "TAB-b": {"children": [], "type": "TAB", "meta": {"text":
"Details"}},
+ }
+ names = _collect_available_tab_names(layout)
+ assert names == ["Overview (TAB-a)", "Details (TAB-b)"]
+
+ def test_collect_available_tab_names_falls_back_to_id(self) -> None:
+ """_collect_available_tab_names uses component ID only when text is
empty."""
+ layout = {
+ "GRID_ID": {"children": ["TABS-x"], "type": "GRID"},
+ "TABS-x": {"children": ["TAB-a"], "type": "TABS"},
+ "TAB-a": {"children": [], "type": "TAB", "meta": {}},
+ }
+ names = _collect_available_tab_names(layout)
+ assert names == ["TAB-a"]
+
+ def test_collect_available_tab_names_duplicate_names(self) -> None:
+ """Duplicate display names are disambiguated by component ID in the
entry."""
+ layout = {
+ "GRID_ID": {"children": ["TABS-x"], "type": "GRID"},
+ "TABS-x": {"children": ["TAB-a", "TAB-b"], "type": "TABS"},
+ "TAB-a": {"children": [], "type": "TAB", "meta": {"text":
"Sales"}},
+ "TAB-b": {"children": [], "type": "TAB", "meta": {"text":
"Sales"}},
+ }
+ names = _collect_available_tab_names(layout)
+ assert names == ["Sales (TAB-a)", "Sales (TAB-b)"]
+ assert names[0] != names[1]
+
+ def test_collect_available_tab_names_no_tabs(self) -> None:
+ """_collect_available_tab_names returns empty list for non-tabbed
dashboards."""
+ layout = {
+ "GRID_ID": {"children": ["ROW-1"], "type": "GRID"},
+ "ROW-1": {"children": [], "type": "ROW"},
+ }
+ assert _collect_available_tab_names(layout) == []
+
def test_add_chart_to_layout_creates_column(self):
"""Test that _add_chart_to_layout creates ROW > COLUMN > CHART."""
layout = {
