This is an automated email from the ASF dual-hosted git repository.
arivero 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 6e94a6c21af fix(mcp): fix dashboard chart placement with proper COLUMN
layout and tab support (#37970)
6e94a6c21af is described below
commit 6e94a6c21af3ffb132a4c386d83cba21e8d1259f
Author: Amin Ghadersohi <[email protected]>
AuthorDate: Mon Feb 23 10:41:10 2026 -0500
fix(mcp): fix dashboard chart placement with proper COLUMN layout and tab
support (#37970)
Co-authored-by: Claude Opus 4.6 <[email protected]>
---
superset/mcp_service/dashboard/constants.py | 38 ++
.../tool/add_chart_to_existing_dashboard.py | 162 +++++++--
.../dashboard/tool/generate_dashboard.py | 42 ++-
.../dashboard/tool/test_dashboard_generation.py | 399 ++++++++++++++++++---
4 files changed, 547 insertions(+), 94 deletions(-)
diff --git a/superset/mcp_service/dashboard/constants.py
b/superset/mcp_service/dashboard/constants.py
new file mode 100644
index 00000000000..6010816eb3e
--- /dev/null
+++ b/superset/mcp_service/dashboard/constants.py
@@ -0,0 +1,38 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Shared constants for MCP dashboard tools.
+
+Values match frontend defaults from
+``superset-frontend/src/dashboard/util/constants.ts``.
+"""
+
+import uuid
+
+GRID_DEFAULT_CHART_WIDTH = 4
+GRID_COLUMN_COUNT = 12
+
+
+def generate_id(prefix: str) -> str:
+ """
+ Generate a component ID matching the frontend's nanoid-style pattern.
+
+ Uses a UUID hex prefix to produce IDs like ``ROW-a1b2c3d4`` which are
+ compatible with the frontend's ``nanoid()``-based ID generation.
+ """
+ return f"{prefix}-{uuid.uuid4().hex[:8]}"
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 86275cfb3ce..74730d799b3 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
@@ -28,6 +28,11 @@ from fastmcp import Context
from superset_core.mcp import tool
from superset.extensions import event_logger
+from superset.mcp_service.dashboard.constants import (
+ generate_id,
+ GRID_COLUMN_COUNT,
+ GRID_DEFAULT_CHART_WIDTH,
+)
from superset.mcp_service.dashboard.schemas import (
AddChartToDashboardRequest,
AddChartToDashboardResponse,
@@ -40,38 +45,95 @@ from superset.utils import json
logger = logging.getLogger(__name__)
-def _find_next_row_position(layout: Dict[str, Any]) -> int:
+def _find_next_row_position(layout: Dict[str, Any]) -> str:
"""
- Find the next available row position in the dashboard layout.
+ Generate a unique ROW ID for a new row in the dashboard layout.
+
+ Uses UUID-based IDs (e.g. ``ROW-a1b2c3d4``) instead of numeric indices
+ so that the IDs are compatible with real dashboard layouts that use
+ nanoid-style identifiers.
Returns:
- Row index for the new chart
+ A new unique ROW ID string.
"""
- # Find existing rows
- row_indices = []
- for key in layout.keys():
- if key.startswith("ROW-") and key[4:].isdigit():
- row_indices.append(int(key[4:]))
+ row_key = generate_id("ROW")
+ # Ensure uniqueness (extremely unlikely collision, but safe)
+ while row_key in layout:
+ row_key = generate_id("ROW")
+ return row_key
+
- # Return next available row index
- return max(row_indices) + 1 if row_indices else 0
+def _find_tab_insert_target(layout: Dict[str, Any]) -> str | None:
+ """
+ Detect if the dashboard uses tabs and return the first tab's ID.
+
+ 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.
+ """
+ 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
+ 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
+ return None
def _add_chart_to_layout(
- layout: Dict[str, Any], chart: Any, chart_id: int, row_index: int
-) -> tuple[str, str]:
+ layout: Dict[str, Any],
+ chart: Any,
+ chart_id: int,
+ row_key: str,
+ parent_id: str,
+) -> tuple[str, str, str]:
"""
- Add chart and row components to the dashboard layout.
+ Add chart, column, and row components to the dashboard layout.
+
+ Creates the proper ``ROW > COLUMN > CHART`` hierarchy that the
+ frontend expects for rendering.
+
+ Args:
+ layout: The mutable layout dict to update.
+ chart: The chart ORM object.
+ chart_id: The chart's integer ID.
+ row_key: The pre-generated ROW component ID.
+ parent_id: The parent container ID (GRID_ID or a TAB ID).
Returns:
- Tuple of (chart_key, row_key)
+ Tuple of ``(chart_key, column_key, row_key)``.
"""
chart_key = f"CHART-{chart_id}"
- row_key = f"ROW-{row_index}"
- chart_width = 5 # Balanced width for good proportions
+ column_key = generate_id("COLUMN")
+ chart_width = GRID_DEFAULT_CHART_WIDTH
chart_height = 50 # Good height for most chart types
- # Add chart to layout using proper Superset structure
+ # Build the parents chain up to the parent container
+ if (parent_component := layout.get(parent_id)) is not None:
+ parent_parents = parent_component.get("parents", [])
+ elif parent_id == "GRID_ID":
+ # Empty layout: GRID_ID will be created by _ensure_layout_structure
+ # with parents=["ROOT_ID"], so mirror that here.
+ parent_parents = ["ROOT_ID"]
+ else:
+ parent_parents = []
+ row_parents = list(parent_parents) + [parent_id]
+ column_parents = row_parents + [row_key]
+ chart_parents = column_parents + [column_key]
+
+ # Add chart component
layout[chart_key] = {
"children": [],
"id": chart_key,
@@ -82,25 +144,45 @@ def _add_chart_to_layout(
"uuid": str(chart.uuid) if chart.uuid else f"chart-{chart_id}",
"width": chart_width,
},
- "parents": ["ROOT_ID", "GRID_ID", row_key],
+ "parents": chart_parents,
"type": "CHART",
}
- # Create row for the chart
- layout[row_key] = {
+ # Add column wrapper (ROW > COLUMN > CHART)
+ layout[column_key] = {
"children": [chart_key],
+ "id": column_key,
+ "meta": {
+ "background": "BACKGROUND_TRANSPARENT",
+ "width": GRID_COLUMN_COUNT,
+ },
+ "parents": column_parents,
+ "type": "COLUMN",
+ }
+
+ # Create row containing the column
+ layout[row_key] = {
+ "children": [column_key],
"id": row_key,
"meta": {"background": "BACKGROUND_TRANSPARENT"},
- "parents": ["ROOT_ID", "GRID_ID"],
+ "parents": row_parents,
"type": "ROW",
}
- return chart_key, row_key
+ return chart_key, column_key, row_key
-def _ensure_layout_structure(layout: Dict[str, Any], row_key: str) -> None:
+def _ensure_layout_structure(
+ layout: Dict[str, Any], row_key: str, parent_id: str
+) -> None:
"""
- Ensure the dashboard layout has proper GRID and ROOT structure.
+ Ensure the dashboard layout has proper GRID and ROOT structure,
+ and add the new row to the correct parent container.
+
+ Args:
+ layout: The mutable layout dict to update.
+ row_key: The ROW component ID to insert.
+ parent_id: The container to add the row to (GRID_ID or a TAB ID).
"""
# Ensure GRID structure exists
if "GRID_ID" not in layout:
@@ -111,10 +193,16 @@ def _ensure_layout_structure(layout: Dict[str, Any],
row_key: str) -> None:
"type": "GRID",
}
- # Add row to GRID
- if "children" not in layout["GRID_ID"]:
- layout["GRID_ID"]["children"] = []
- layout["GRID_ID"]["children"].append(row_key)
+ # Add row to the target parent container
+ if parent := layout.get(parent_id):
+ if "children" not in parent:
+ parent["children"] = []
+ parent["children"].append(row_key)
+ else:
+ # Fallback: add to GRID_ID
+ if "children" not in layout["GRID_ID"]:
+ layout["GRID_ID"]["children"] = []
+ layout["GRID_ID"]["children"].append(row_key)
# Update ROOT_ID if it exists, or create it
if "ROOT_ID" in layout:
@@ -193,16 +281,20 @@ def add_chart_to_existing_dashboard(
except (json.JSONDecodeError, TypeError):
current_layout = {}
- # Find position for new chart
- row_index = _find_next_row_position(current_layout)
+ # 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)
+ parent_id = tab_target if tab_target else "GRID_ID"
- # Add chart and row to layout
- chart_key, row_key = _add_chart_to_layout(
- current_layout, new_chart, request.chart_id, row_index
+ # Add chart, column, and row to layout
+ chart_key, column_key, row_key = _add_chart_to_layout(
+ current_layout, new_chart, request.chart_id, row_key, parent_id
)
# Ensure proper layout structure
- _ensure_layout_structure(current_layout, row_key)
+ _ensure_layout_structure(current_layout, row_key, parent_id)
# Update the dashboard
with
event_logger.log_context(action="mcp.add_chart_to_dashboard.db_write"):
@@ -268,7 +360,7 @@ def add_chart_to_existing_dashboard(
)
# Return position info for compatibility
- position_info = {"row": row_index, "chart_key": chart_key, "row_key":
row_key}
+ position_info = {"row": row_key, "chart_key": chart_key, "row_key":
row_key}
return AddChartToDashboardResponse(
dashboard=dashboard_info,
diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py
b/superset/mcp_service/dashboard/tool/generate_dashboard.py
index 5d537ced2af..29cfb61eef0 100644
--- a/superset/mcp_service/dashboard/tool/generate_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py
@@ -28,6 +28,11 @@ from fastmcp import Context
from superset_core.mcp import tool
from superset.extensions import event_logger
+from superset.mcp_service.dashboard.constants import (
+ generate_id,
+ GRID_COLUMN_COUNT,
+ GRID_DEFAULT_CHART_WIDTH,
+)
from superset.mcp_service.dashboard.schemas import (
DashboardInfo,
GenerateDashboardRequest,
@@ -44,8 +49,8 @@ def _create_dashboard_layout(chart_objects: List[Any]) ->
Dict[str, Any]:
"""
Create a simple dashboard layout with charts arranged in a grid.
- This creates a basic 2-column layout where charts are arranged
- vertically in alternating columns.
+ This creates a ``ROW > COLUMN > CHART`` hierarchy for each row,
+ matching the component structure that the Superset frontend expects.
Args:
chart_objects: List of Chart ORM objects (not IDs)
@@ -55,23 +60,26 @@ def _create_dashboard_layout(chart_objects: List[Any]) ->
Dict[str, Any]:
# Grid configuration based on real Superset dashboard patterns
# Use 2-chart rows with medium-sized charts (like existing dashboards)
charts_per_row = 2
- chart_width = 5 # Balanced width for good proportions
+ chart_width = GRID_DEFAULT_CHART_WIDTH
chart_height = 50 # Good height for most chart types
- # Create rows with charts
+ # Create rows with charts wrapped in columns
row_ids = []
for i in range(0, len(chart_objects), charts_per_row):
- row_index = i // charts_per_row
- row_id = f"ROW-{row_index}"
+ row_id = generate_id("ROW")
row_ids.append(row_id)
# Get charts for this row (up to 2 charts like real dashboards)
row_charts = chart_objects[i : i + charts_per_row]
- chart_keys = []
+ column_keys = []
+
+ # Calculate column width: divide grid evenly among charts in this row
+ col_width = GRID_COLUMN_COUNT // len(row_charts)
for chart in row_charts:
chart_key = f"CHART-{chart.id}"
- chart_keys.append(chart_key)
+ column_key = generate_id("COLUMN")
+ column_keys.append(column_key)
# Create chart component with standard dimensions
layout[chart_key] = {
@@ -84,13 +92,25 @@ def _create_dashboard_layout(chart_objects: List[Any]) ->
Dict[str, Any]:
"uuid": str(chart.uuid) if chart.uuid else
f"chart-{chart.id}",
"width": chart_width,
},
- "parents": ["ROOT_ID", "GRID_ID", row_id],
+ "parents": ["ROOT_ID", "GRID_ID", row_id, column_key],
"type": "CHART",
}
- # Create row containing the charts
+ # Create column wrapper for the chart (ROW > COLUMN > CHART)
+ layout[column_key] = {
+ "children": [chart_key],
+ "id": column_key,
+ "meta": {
+ "background": "BACKGROUND_TRANSPARENT",
+ "width": col_width,
+ },
+ "parents": ["ROOT_ID", "GRID_ID", row_id],
+ "type": "COLUMN",
+ }
+
+ # Create row containing the columns
layout[row_id] = {
- "children": chart_keys,
+ "children": column_keys,
"id": row_id,
"meta": {"background": "BACKGROUND_TRANSPARENT"},
"parents": ["ROOT_ID", "GRID_ID"],
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 32d9dda92ee..83fe00df0fb 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
@@ -26,6 +26,13 @@ import pytest
from fastmcp import Client
from superset.mcp_service.app import mcp
+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,
+ _ensure_layout_structure,
+ _find_next_row_position,
+ _find_tab_insert_target,
+)
from superset.utils import json
logging.basicConfig(level=logging.DEBUG)
@@ -73,8 +80,8 @@ def _mock_dashboard(id: int = 1, title: str = "Test
Dashboard") -> Mock:
dashboard.changed_by.username = "test_user"
dashboard.uuid = f"dashboard-uuid-{id}"
dashboard.slices = []
- dashboard.owners = [] # Add missing owners attribute
- dashboard.tags = [] # Add missing tags attribute
+ dashboard.owners = []
+ dashboard.tags = []
return dashboard
@@ -88,7 +95,6 @@ class TestGenerateDashboard:
self, mock_db_session, mock_create_command, mcp_server
):
"""Test basic dashboard generation with valid charts."""
- # Mock database query for charts
mock_query = Mock()
mock_filter = Mock()
mock_query.filter.return_value = mock_filter
@@ -98,7 +104,6 @@ class TestGenerateDashboard:
]
mock_db_session.query.return_value = mock_query
- # Mock dashboard creation
mock_dashboard = _mock_dashboard(id=10, title="Analytics Dashboard")
mock_create_command.return_value.run.return_value = mock_dashboard
@@ -128,14 +133,10 @@ class TestGenerateDashboard:
@pytest.mark.asyncio
async def test_generate_dashboard_missing_charts(self, mock_db_session,
mcp_server):
"""Test error handling when some charts don't exist."""
- # Mock database query returning only chart 1 (chart 2 missing)
mock_query = Mock()
mock_filter = Mock()
mock_query.filter.return_value = mock_filter
- mock_filter.all.return_value = [
- _mock_chart(id=1),
- # Chart 2 is missing from the result
- ]
+ mock_filter.all.return_value = [_mock_chart(id=1)]
mock_db_session.query.return_value = mock_query
request = {"chart_ids": [1, 2], "dashboard_title": "Test Dashboard"}
@@ -155,7 +156,6 @@ class TestGenerateDashboard:
self, mock_db_session, mock_create_command, mcp_server
):
"""Test dashboard generation with a single chart."""
- # Mock database query for single chart
mock_query = Mock()
mock_filter = Mock()
mock_query.filter.return_value = mock_filter
@@ -185,7 +185,6 @@ class TestGenerateDashboard:
self, mock_db_session, mock_create_command, mcp_server
):
"""Test dashboard generation with many charts (grid layout)."""
- # Mock 6 charts
chart_ids = list(range(1, 7))
mock_query = Mock()
mock_filter = Mock()
@@ -206,42 +205,48 @@ class TestGenerateDashboard:
assert result.structured_content["error"] is None
assert result.structured_content["dashboard"]["chart_count"] == 6
- # Verify CreateDashboardCommand was called with proper layout
mock_create_command.assert_called_once()
call_args = mock_create_command.call_args[0][0]
- # Check position_json contains proper layout
position_json = json.loads(call_args["position_json"])
assert "ROOT_ID" in position_json
assert "GRID_ID" in position_json
assert "DASHBOARD_VERSION_KEY" in position_json
assert position_json["DASHBOARD_VERSION_KEY"] == "v2"
- # ROOT should only contain GRID
assert position_json["ROOT_ID"]["children"] == ["GRID_ID"]
- # GRID should contain rows (6 charts = 3 rows in 2-chart layout)
grid_children = position_json["GRID_ID"]["children"]
assert len(grid_children) == 3
- # Check each chart has proper structure
- for i, chart_id in enumerate(chart_ids):
+ for row_id in grid_children:
+ assert row_id.startswith("ROW-")
+
+ for chart_id in chart_ids:
chart_key = f"CHART-{chart_id}"
- row_index = i // 2 # 2 charts per row
- row_key = f"ROW-{row_index}"
- # Chart should exist
assert chart_key in position_json
chart_data = position_json[chart_key]
assert chart_data["type"] == "CHART"
assert "meta" in chart_data
assert chart_data["meta"]["chartId"] == chart_id
-
- # Row should exist and contain charts (up to 2 per row)
+ assert chart_data["meta"]["width"] == 4
+
+ chart_parents = chart_data["parents"]
+ column_key = chart_parents[-1]
+ assert column_key.startswith("COLUMN-")
+ assert column_key in position_json
+ column_data = position_json[column_key]
+ assert column_data["type"] == "COLUMN"
+ assert chart_key in column_data["children"]
+
+ column_parents = column_data["parents"]
+ row_key = column_parents[-1]
+ assert row_key.startswith("ROW-")
assert row_key in position_json
row_data = position_json[row_key]
assert row_data["type"] == "ROW"
- assert chart_key in row_data["children"]
+ assert column_key in row_data["children"]
@patch("superset.commands.dashboard.create.CreateDashboardCommand")
@patch("superset.db.session")
@@ -273,7 +278,6 @@ class TestGenerateDashboard:
self, mock_db_session, mock_create_command, mcp_server
):
"""Test dashboard generation with minimal required parameters."""
- # Mock database query for single chart
mock_query = Mock()
mock_filter = Mock()
mock_query.filter.return_value = mock_filter
@@ -286,7 +290,6 @@ class TestGenerateDashboard:
request = {
"chart_ids": [3],
"dashboard_title": "Minimal Dashboard",
- # No description, published defaults to True
}
async with Client(mcp_server) as client:
@@ -298,9 +301,8 @@ class TestGenerateDashboard:
== "Minimal Dashboard"
)
- # Check that description was not included in call
call_args = mock_create_command.call_args[0][0]
- assert call_args["published"] is True # Default value
+ assert call_args["published"] is True
assert (
"description" not in call_args or call_args.get("description")
is None
)
@@ -317,29 +319,46 @@ class TestAddChartToExistingDashboard:
self, mock_db_session, mock_find_dashboard, mock_update_command,
mcp_server
):
"""Test adding a chart to an existing dashboard."""
- # Mock existing dashboard with some charts
mock_dashboard = _mock_dashboard(id=1, title="Existing Dashboard")
- mock_dashboard.slices = [Mock(id=10), Mock(id=20)] # Existing charts
+ mock_dashboard.slices = [Mock(id=10), Mock(id=20)]
mock_dashboard.position_json = json.dumps(
{
"ROOT_ID": {
- "children": ["CHART-10", "CHART-20"],
+ "children": ["GRID_ID"],
"id": "ROOT_ID",
"type": "ROOT",
},
- "CHART-10": {"id": "CHART-10", "type": "CHART", "parents":
["ROOT_ID"]},
- "CHART-10_POSITION": {"h": 16, "w": 24, "x": 0, "y": 0},
- "CHART-20": {"id": "CHART-20", "type": "CHART", "parents":
["ROOT_ID"]},
- "CHART-20_POSITION": {"h": 16, "w": 24, "x": 24, "y": 0},
+ "GRID_ID": {
+ "children": ["ROW-abc123"],
+ "id": "GRID_ID",
+ "parents": ["ROOT_ID"],
+ "type": "GRID",
+ },
+ "ROW-abc123": {
+ "children": ["CHART-10", "CHART-20"],
+ "id": "ROW-abc123",
+ "meta": {"background": "BACKGROUND_TRANSPARENT"},
+ "parents": ["ROOT_ID", "GRID_ID"],
+ "type": "ROW",
+ },
+ "CHART-10": {
+ "id": "CHART-10",
+ "type": "CHART",
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-abc123"],
+ },
+ "CHART-20": {
+ "id": "CHART-20",
+ "type": "CHART",
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-abc123"],
+ },
+ "DASHBOARD_VERSION_KEY": "v2",
}
)
mock_find_dashboard.return_value = mock_dashboard
- # Mock chart to add
mock_chart = _mock_chart(id=30, slice_name="New Chart")
mock_db_session.get.return_value = mock_chart
- # Mock updated dashboard
updated_dashboard = _mock_dashboard(id=1, title="Existing Dashboard")
updated_dashboard.slices = [Mock(id=10), Mock(id=20), Mock(id=30)]
mock_update_command.return_value.run.return_value = updated_dashboard
@@ -357,23 +376,34 @@ class TestAddChartToExistingDashboard:
assert result.structured_content["position"] is not None
assert "row" in result.structured_content["position"]
assert "chart_key" in result.structured_content["position"]
+ row_key = result.structured_content["position"]["row_key"]
+ assert row_key.startswith("ROW-")
assert (
"/superset/dashboard/1/" in
result.structured_content["dashboard_url"]
)
+ call_args = mock_update_command.call_args[0][1]
+ layout = json.loads(call_args["position_json"])
+ assert "CHART-30" in layout
+ chart_data = layout["CHART-30"]
+ assert chart_data["type"] == "CHART"
+ chart_parents = chart_data["parents"]
+ column_key = chart_parents[-1]
+ assert column_key.startswith("COLUMN-")
+ assert column_key in layout
+ assert layout[column_key]["type"] == "COLUMN"
+
@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
@pytest.mark.asyncio
async def test_add_chart_dashboard_not_found(self, mock_find_dashboard,
mcp_server):
"""Test error when dashboard doesn't exist."""
mock_find_dashboard.return_value = None
-
request = {"dashboard_id": 999, "chart_id": 1}
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
assert (
"Dashboard with ID 999 not found" in
result.structured_content["error"]
@@ -388,14 +418,12 @@ class TestAddChartToExistingDashboard:
"""Test error when chart doesn't exist."""
mock_find_dashboard.return_value = _mock_dashboard()
mock_db_session.get.return_value = None
-
request = {"dashboard_id": 1, "chart_id": 999}
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
assert "Chart with ID 999 not found" in
result.structured_content["error"]
@@ -407,18 +435,15 @@ class TestAddChartToExistingDashboard:
):
"""Test error when chart is already in dashboard."""
mock_dashboard = _mock_dashboard()
- mock_dashboard.slices = [Mock(id=5)] # Chart 5 already exists
+ mock_dashboard.slices = [Mock(id=5)]
mock_find_dashboard.return_value = mock_dashboard
-
mock_db_session.get.return_value = _mock_chart(id=5)
-
request = {"dashboard_id": 1, "chart_id": 5}
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
assert (
"Chart 5 is already in dashboard 1"
@@ -435,7 +460,7 @@ class TestAddChartToExistingDashboard:
"""Test adding chart to dashboard with no existing layout."""
mock_dashboard = _mock_dashboard(id=2)
mock_dashboard.slices = []
- mock_dashboard.position_json = "{}" # Empty layout
+ mock_dashboard.position_json = "{}"
mock_find_dashboard.return_value = mock_dashboard
mock_chart = _mock_chart(id=15)
@@ -454,12 +479,290 @@ class TestAddChartToExistingDashboard:
assert result.structured_content["error"] is None
assert "row" in result.structured_content["position"]
- assert result.structured_content["position"].get("row") == 0
+ row_key = result.structured_content["position"]["row"]
+ assert isinstance(row_key, str)
+ assert row_key.startswith("ROW-")
- # Verify update was called with proper layout structure
call_args = mock_update_command.call_args[0][1]
layout = json.loads(call_args["position_json"])
assert "ROOT_ID" in layout
assert "GRID_ID" in layout
- assert "ROW-0" in layout
assert "CHART-15" in layout
+
+ assert row_key in layout
+ row_data = layout[row_key]
+ assert row_data["type"] == "ROW"
+ assert len(row_data["children"]) == 1
+
+ column_key = row_data["children"][0]
+ assert column_key.startswith("COLUMN-")
+ assert column_key in layout
+ column_data = layout[column_key]
+ assert column_data["type"] == "COLUMN"
+ assert "CHART-15" in column_data["children"]
+
+ # Verify ROOT_ID is in parents chain even for empty layouts
+ chart_parents = layout["CHART-15"]["parents"]
+ assert "ROOT_ID" in chart_parents
+ assert "GRID_ID" 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_tabbed_dashboard(
+ self, mock_db_session, mock_find_dashboard, mock_update_command,
mcp_server
+ ):
+ """Test adding chart to a dashboard that uses tabs."""
+ 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": "Tab 1"},
+ "parents": ["ROOT_ID", "GRID_ID", "TABS-abc123"],
+ "type": "TAB",
+ },
+ "TAB-tab2": {
+ "children": [],
+ "id": "TAB-tab2",
+ "meta": {"text": "Tab 2"},
+ "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=25, slice_name="Tab 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=25)]
+ mock_update_command.return_value.run.return_value = updated_dashboard
+
+ request = {"dashboard_id": 3, "chart_id": 25}
+
+ 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
+ assert row_key in layout["TAB-tab1"]["children"]
+ assert layout["GRID_ID"]["children"] == ["TABS-abc123"]
+
+ row_data = layout[row_key]
+ assert row_data["type"] == "ROW"
+ column_key = row_data["children"][0]
+ assert layout[column_key]["type"] == "COLUMN"
+ assert "CHART-25" in layout[column_key]["children"]
+
+ chart_parents = layout["CHART-25"]["parents"]
+ 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_dashboard_with_nanoid_rows(
+ self, mock_db_session, mock_find_dashboard, mock_update_command,
mcp_server
+ ):
+ """Test adding chart to dashboard that has nanoid-style ROW IDs."""
+ mock_dashboard = _mock_dashboard(id=4, title="Nanoid 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": ["ROW-46632bc2"],
+ "id": "GRID_ID",
+ "parents": ["ROOT_ID"],
+ "type": "GRID",
+ },
+ "ROW-46632bc2": {
+ "children": ["CHART-10"],
+ "id": "ROW-46632bc2",
+ "meta": {"background": "BACKGROUND_TRANSPARENT"},
+ "parents": ["ROOT_ID", "GRID_ID"],
+ "type": "ROW",
+ },
+ "CHART-10": {
+ "id": "CHART-10",
+ "type": "CHART",
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-46632bc2"],
+ },
+ "DASHBOARD_VERSION_KEY": "v2",
+ }
+ )
+ mock_find_dashboard.return_value = mock_dashboard
+
+ mock_chart = _mock_chart(id=50, slice_name="New Nanoid Chart")
+ mock_db_session.get.return_value = mock_chart
+
+ updated_dashboard = _mock_dashboard(id=4, title="Nanoid Dashboard")
+ updated_dashboard.slices = [Mock(id=10), Mock(id=50)]
+ mock_update_command.return_value.run.return_value = updated_dashboard
+
+ request = {"dashboard_id": 4, "chart_id": 50}
+
+ 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 != "ROW-46632bc2"
+ assert row_key.startswith("ROW-")
+ assert row_key in layout
+
+
+class TestLayoutHelpers:
+ """Tests for layout helper functions."""
+
+ def test_generate_id_format(self):
+ """Test that _generate_id produces correct format."""
+ row_id = generate_id("ROW")
+ assert row_id.startswith("ROW-")
+ assert len(row_id) == 12
+
+ col_id = generate_id("COLUMN")
+ assert col_id.startswith("COLUMN-")
+ assert len(col_id) == 15
+
+ def test_generate_id_uniqueness(self):
+ """Test that _generate_id produces unique IDs."""
+ ids = {generate_id("ROW") for _ in range(100)}
+ assert len(ids) == 100
+
+ def test_find_next_row_position_empty_layout(self):
+ """Test _find_next_row_position with empty layout."""
+ result = _find_next_row_position({})
+ assert isinstance(result, str)
+ assert result.startswith("ROW-")
+
+ def test_find_tab_insert_target_no_tabs(self):
+ """Test _find_tab_insert_target with no tabs."""
+ layout = {"GRID_ID": {"children": ["ROW-1"], "type": "GRID"}}
+ assert _find_tab_insert_target(layout) is None
+
+ def test_find_tab_insert_target_with_tabs(self):
+ """Test _find_tab_insert_target with tabbed dashboard."""
+ layout = {
+ "GRID_ID": {"children": ["TABS-main"], "type": "GRID"},
+ "TABS-main": {"children": ["TAB-first", "TAB-second"], "type":
"TABS"},
+ "TAB-first": {"children": [], "type": "TAB"},
+ "TAB-second": {"children": [], "type": "TAB"},
+ }
+ assert _find_tab_insert_target(layout) == "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
+
+ def test_add_chart_to_layout_creates_column(self):
+ """Test that _add_chart_to_layout creates ROW > COLUMN > CHART."""
+ layout = {
+ "GRID_ID": {"children": [], "parents": ["ROOT_ID"], "type":
"GRID"},
+ }
+ chart = Mock()
+ chart.slice_name = "Test Chart"
+ chart.uuid = "test-uuid"
+
+ chart_key, column_key, row_key = _add_chart_to_layout(
+ layout, chart, 42, "ROW-test123", "GRID_ID"
+ )
+
+ assert chart_key == "CHART-42"
+ assert column_key.startswith("COLUMN-")
+ assert row_key == "ROW-test123"
+
+ assert layout[row_key]["type"] == "ROW"
+ assert layout[row_key]["children"] == [column_key]
+ assert layout[column_key]["type"] == "COLUMN"
+ assert layout[column_key]["children"] == [chart_key]
+ assert layout[column_key]["meta"]["width"] == 12
+ assert layout[chart_key]["type"] == "CHART"
+ assert layout[chart_key]["meta"]["width"] == 4
+ assert layout[chart_key]["meta"]["chartId"] == 42
+
+ def test_ensure_layout_structure_creates_missing(self):
+ """Test _ensure_layout_structure creates GRID and ROOT if missing."""
+ layout: dict = {}
+ _ensure_layout_structure(layout, "ROW-test", "GRID_ID")
+
+ assert "ROOT_ID" in layout
+ assert "GRID_ID" in layout
+ assert "GRID_ID" in layout["ROOT_ID"]["children"]
+ assert "ROW-test" in layout["GRID_ID"]["children"]
+ assert layout["DASHBOARD_VERSION_KEY"] == "v2"
+
+ def test_ensure_layout_structure_adds_to_tab(self):
+ """Test _ensure_layout_structure adds row to tab parent."""
+ layout = {
+ "ROOT_ID": {"children": ["GRID_ID"], "type": "ROOT"},
+ "GRID_ID": {
+ "children": ["TABS-main"],
+ "parents": ["ROOT_ID"],
+ "type": "GRID",
+ },
+ "TAB-first": {"children": ["ROW-existing"], "type": "TAB"},
+ }
+ _ensure_layout_structure(layout, "ROW-new", "TAB-first")
+
+ assert "ROW-new" in layout["TAB-first"]["children"]
+ assert "ROW-new" not in layout["GRID_ID"]["children"]
