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 48b21790465..2ffec57f267 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 @@ -22,6 +22,7 @@ This tool adds a chart to an existing dashboard with automatic layout positionin """ import logging +import re from typing import Any, Dict from fastmcp import Context @@ -44,6 +45,22 @@ from superset.utils import json logger = logging.getLogger(__name__) +# Compiled regex for stripping common emoji Unicode ranges from tab text. +# Uses specific Unicode blocks to avoid overly permissive ranges. +_EMOJI_RE = re.compile( + "[" + "\U0001f300-\U0001f5ff" # Misc Symbols and Pictographs + "\U0001f600-\U0001f64f" # Emoticons + "\U0001f680-\U0001f6ff" # Transport and Map Symbols + "\U0001f900-\U0001f9ff" # Supplemental Symbols and Pictographs + "\U0001fa70-\U0001faff" # Symbols and Pictographs Extended-A + "\u2600-\u26ff" # Misc Symbols + "\u2700-\u27bf" # Dingbats + "\ufe00-\ufe0f" # Variation Selectors + "\u200d" # Zero-width joiner + "]+" +) + def _find_next_row_position(layout: Dict[str, Any]) -> str: """ @@ -63,35 +80,99 @@ def _find_next_row_position(layout: Dict[str, Any]) -> str: return row_key -def _find_tab_insert_target(layout: Dict[str, Any]) -> str | None: +def _normalize_tab_text(text: str | None) -> str: + """Strip emoji and extra whitespace from tab text for flexible matching.""" + if not text: + return "" + cleaned = _EMOJI_RE.sub("", text) + return cleaned.strip().lower() + + +def _match_tab_in_children( + layout: Dict[str, Any], + tabs_children: list[str], + target_tab: str, +) -> str | None: + """Search tabs_children for a tab matching target_tab by ID or name. + + Matching is flexible: exact ID match, exact text match, or + case-insensitive text match after stripping emoji characters. """ - Detect if the dashboard uses tabs and return the first tab's ID. + target_normalized = _normalize_tab_text(target_tab) + for tab_id in tabs_children: + tab = layout.get(tab_id) + if not tab or tab.get("type") != "TAB": + continue + tab_text = (tab.get("meta") or {}).get("text", "") + # Exact match on ID or text + if target_tab in (tab_id, tab_text): + return tab_id + # Flexible match: case-insensitive, emoji-stripped + if target_normalized and _normalize_tab_text(tab_text) == target_normalized: + return tab_id + return None - If ``GRID_ID`` has children that are ``TABS`` components, this walks - into the first ``TAB`` child so that new rows are placed inside the - active tab rather than directly under GRID_ID. - Returns: - The ID of the first TAB component, or ``None`` if the dashboard - does not use top-level tabs. +def _collect_tabs_groups(layout: Dict[str, Any]) -> list[list[str]]: + """Collect all TABS groups from ROOT_ID and GRID_ID children. + + Superset dashboards can place TABS under either ROOT_ID or GRID_ID + depending on how the layout was constructed. """ - grid = layout.get("GRID_ID") - if not grid: - return None - - for child_id in grid.get("children", []): - child = layout.get(child_id) - if child and child.get("type") == "TABS": - # Found a TABS component; use its first TAB child + groups: list[list[str]] = [] + for parent_key in ("ROOT_ID", "GRID_ID"): + parent = layout.get(parent_key) + if not parent: + continue + for child_id in parent.get("children", []): + child = layout.get(child_id) + if not child or child.get("type") != "TABS": + continue tabs_children = child.get("children", []) if tabs_children: - first_tab_id = tabs_children[0] - first_tab = layout.get(first_tab_id) - if first_tab and first_tab.get("type") == "TAB": - return first_tab_id + groups.append(tabs_children) + return groups + + +def _first_tab_from_groups( + layout: Dict[str, Any], groups: list[list[str]] +) -> str | None: + """Return the first valid TAB ID from the collected groups.""" + for tabs_children in groups: + first_tab_id = tabs_children[0] + first_tab = layout.get(first_tab_id) + if first_tab and first_tab.get("type") == "TAB": + return first_tab_id return None +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``. + + Returns: + The ID of the matched (or first) TAB component, or ``None`` if the + dashboard does not use top-level tabs. + """ + groups = _collect_tabs_groups(layout) + + if target_tab: + for tabs_children in groups: + matched = _match_tab_in_children(layout, tabs_children, target_tab) + if matched: + return matched + + return _first_tab_from_groups(layout, groups) + + def _add_chart_to_layout( layout: Dict[str, Any], chart: Any, @@ -284,8 +365,10 @@ 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: if GRID has TABS, target the first tab - tab_target = _find_tab_insert_target(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 + ) parent_id = tab_target if tab_target else "GRID_ID" # Add chart, column, and row to layout 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 a0d3091744e..912bb74fc08 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 @@ -676,6 +676,107 @@ class TestAddChartToExistingDashboard: assert "TABS-abc123" in chart_parents assert "TAB-tab1" 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_to_specific_tab_by_name( + self, mock_db_session, mock_find_dashboard, mock_update_command, mcp_server + ): + """Test adding chart to a specific tab using target_tab name.""" + mock_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard") + mock_dashboard.slices = [Mock(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": ["ROW-existing"], + "id": "TAB-tab1", + "meta": {"text": "Activity Metrics"}, + "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"], + "type": "TAB", + }, + "TAB-tab2": { + "children": [], + "id": "TAB-tab2", + "meta": {"text": "Customers"}, + "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"], + "type": "TAB", + }, + "ROW-existing": { + "children": ["CHART-10"], + "id": "ROW-existing", + "meta": {"background": "BACKGROUND_TRANSPARENT"}, + "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123", "TAB-tab1"], + "type": "ROW", + }, + "CHART-10": { + "id": "CHART-10", + "type": "CHART", + "parents": [ + "ROOT_ID", + "GRID_ID", + "TABS-abc123", + "TAB-tab1", + "ROW-existing", + ], + }, + "DASHBOARD_VERSION_KEY": "v2", + } + ) + mock_find_dashboard.return_value = mock_dashboard + + mock_chart = _mock_chart(id=30, slice_name="Customer Chart") + mock_db_session.get.return_value = mock_chart + + updated_dashboard = _mock_dashboard(id=3, title="Tabbed Dashboard") + updated_dashboard.slices = [Mock(id=10), Mock(id=30)] + mock_update_command.return_value.run.return_value = updated_dashboard + + request = {"dashboard_id": 3, "chart_id": 30, "target_tab": "Customers"} + + 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 None + + call_args = mock_update_command.call_args[0][1] + layout = json.loads(call_args["position_json"]) + + row_key = result.structured_content["position"]["row_key"] + assert row_key in layout + + # Chart should be in TAB-tab2 (Customers), NOT TAB-tab1 + assert row_key in layout["TAB-tab2"]["children"] + assert row_key not in layout["TAB-tab1"]["children"] + + # GRID_ID should still only have TABS, not the new row + assert layout["GRID_ID"]["children"] == ["TABS-abc123"] + + # Verify correct parent chain includes TAB-tab2 + chart_parents = layout["CHART-30"]["parents"] + assert "TABS-abc123" in chart_parents + 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") @@ -780,6 +881,63 @@ class TestLayoutHelpers: } assert _find_tab_insert_target(layout) == "TAB-first" + def test_find_tab_insert_target_by_tab_name(self): + """Test _find_tab_insert_target resolves target_tab by display name.""" + layout = { + "GRID_ID": {"children": ["TABS-main"], "type": "GRID"}, + "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"}, + "TAB-first": { + "children": [], + "type": "TAB", + "meta": {"text": "Activity Metrics"}, + }, + "TAB-second": { + "children": [], + "type": "TAB", + "meta": {"text": "Customers"}, + }, + } + assert _find_tab_insert_target(layout, target_tab="Customers") == "TAB-second" + + def test_find_tab_insert_target_by_tab_id(self): + """Test _find_tab_insert_target resolves target_tab by component ID.""" + layout = { + "GRID_ID": {"children": ["TABS-main"], "type": "GRID"}, + "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"}, + "TAB-first": { + "children": [], + "type": "TAB", + "meta": {"text": "Tab 1"}, + }, + "TAB-second": { + "children": [], + "type": "TAB", + "meta": {"text": "Tab 2"}, + }, + } + 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.""" + layout = { + "GRID_ID": {"children": ["TABS-main"], "type": "GRID"}, + "TABS-main": {"children": ["TAB-first", "TAB-second"], "type": "TABS"}, + "TAB-first": { + "children": [], + "type": "TAB", + "meta": {"text": "Tab 1"}, + }, + "TAB-second": { + "children": [], + "type": "TAB", + "meta": {"text": "Tab 2"}, + }, + } + assert ( + _find_tab_insert_target(layout, target_tab="Nonexistent Tab") == "TAB-first" + ) + def test_find_tab_insert_target_no_grid(self): """Test _find_tab_insert_target with missing GRID_ID.""" assert _find_tab_insert_target({"ROOT_ID": {"type": "ROOT"}}) is None