bito-code-review[bot] commented on code in PR #44560:
URL: https://github.com/apache/superset/pull/44560#discussion_r4117150968
##########
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:
##########
@@ -36,6 +35,12 @@
GRID_COLUMN_COUNT,
GRID_DEFAULT_CHART_WIDTH,
)
+from superset.mcp_service.dashboard.layout_placement import (
+ _collect_available_tab_names,
+ _ensure_layout_structure,
+ _find_next_row_position,
+ _find_tab_insert_target,
+)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Private-symbol cross-module import</b></div>
<div id="fix">
This imports four `_`-prefixed names from `layout_placement`, making
private-by-convention symbols the de-facto API for three tool modules
(`add_chart_to_existing_dashboard`, `remove_chart_from_dashboard`,
`manage_dashboard_markdown`). Counted: 42 underscore-name imports vs 210
cross-module paren imports in `mcp_service` (~20%). Rename to public helpers so
`layout_placement` owns a stable, intentional surface. ([PEP
8](https://peps.python.org/pep-0008/#descriptive-naming-styles))
</div>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/2cb3e84/.cursor/rules/dev-standard.mdc#L119";>dev-standard.mdc:119</a>
</li>
</ul>
</details>
<small><i>Code Review Run #e3ca1e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:
##########
@@ -35,6 +35,9 @@
from superset.commands.exceptions import CommandException, ForbiddenError
from superset.extensions import event_logger
+from superset.mcp_service.dashboard.layout_placement import (
+ _remove_component_and_prune,
+)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Private cross-module import</b></div>
<div id="fix">
This import reaches for the underscore-private helper
`_remove_component_and_prune` from `layout_placement`, turning a private symbol
into a cross-module contract (shared with `manage_dashboard_markdown`). Any
rename/refactor of the helper silently breaks both consumers. Consider exposing
a public name (e.g. `remove_component_and_prune`) in `layout_placement` and
importing that.
</div>
</div>
<small><i>Code Review Run #e3ca1e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2344,6 +2348,246 @@ class ManageNativeFiltersResponse(BaseModel):
)
+# ---------------------------------------------------------------------------
+# manage_dashboard_markdown schemas
+# ---------------------------------------------------------------------------
+
+
+class BaseNewDashboardComponentSpec(BaseModel):
+ """Common placement fields shared by all new markdown/header/divider
specs."""
+
+ model_config = ConfigDict(extra="forbid")
+
+ target_tab: str | None = Field(
+ None,
+ description=(
+ "Tab to add the component to, matched by display name or "
+ "component ID (see get_dashboard_layout for available tabs). "
+ "Omit to use the first tab, or the grid if there are no tabs; "
+ "specify a target when the component should land in a "
+ "specific one rather than the first tab."
+ ),
+ )
+
+
+class MarkdownComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new markdown/text tile.
+
+ Placed in its own new row (a MARKDOWN component sits alongside charts,
+ not as a full-width band), so it composes with existing rows/charts on
+ the target grid or tab.
+ """
+
+ component_type: Literal["markdown"] = Field(
+ ..., description="Discriminator - must be 'markdown'"
+ )
+ code: str = Field(
+ ..., min_length=1, description="Markdown (and safe inline HTML) source"
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-79: Unsanitized Markdown HTML</b></div>
<div id="fix">
`MarkdownComponentSpec.code` is persisted verbatim into position_json meta
(`_build_component_meta` in manage_dashboard_markdown.py) and rendered as HTML
on the dashboard, yet unlike `HeaderComponentSpec.text` it runs no
`sanitize_user_input` check. Its own description promises "safe inline HTML"
but nothing enforces it, so LLM-supplied markup becomes stored XSS. Apply an
HTML allowlist sanitizer before persisting.
([CWE-79](https://cwe.mitre.org/data/definitions/79.html))
</div>
</div>
<small><i>Code Review Run #e3ca1e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -2344,6 +2348,246 @@ class ManageNativeFiltersResponse(BaseModel):
)
+# ---------------------------------------------------------------------------
+# manage_dashboard_markdown schemas
+# ---------------------------------------------------------------------------
+
+
+class BaseNewDashboardComponentSpec(BaseModel):
+ """Common placement fields shared by all new markdown/header/divider
specs."""
+
+ model_config = ConfigDict(extra="forbid")
+
+ target_tab: str | None = Field(
+ None,
+ description=(
+ "Tab to add the component to, matched by display name or "
+ "component ID (see get_dashboard_layout for available tabs). "
+ "Omit to use the first tab, or the grid if there are no tabs; "
+ "specify a target when the component should land in a "
+ "specific one rather than the first tab."
+ ),
+ )
+
+
+class MarkdownComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new markdown/text tile.
+
+ Placed in its own new row (a MARKDOWN component sits alongside charts,
+ not as a full-width band), so it composes with existing rows/charts on
+ the target grid or tab.
+ """
+
+ component_type: Literal["markdown"] = Field(
+ ..., description="Discriminator - must be 'markdown'"
+ )
+ code: str = Field(
+ ..., min_length=1, description="Markdown (and safe inline HTML) source"
+ )
+ width: int = Field(
+ GRID_DEFAULT_CHART_WIDTH,
+ ge=1,
+ le=GRID_COLUMN_COUNT,
+ description=(
+ f"Tile width in grid columns (1-{GRID_COLUMN_COUNT}, "
+ f"default {GRID_DEFAULT_CHART_WIDTH})"
+ ),
+ )
+ height: int = Field(
+ 50,
+ ge=1,
+ description="Tile height in grid units (one unit is 8 pixels; default
50)",
+ )
+
+
+class HeaderComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new section header band.
+
+ Placed directly on the target grid/tab (not inside a row) so it spans
+ the full dashboard width, matching how the dashboard builder places
+ dragged header components.
+ """
+
+ component_type: Literal["header"] = Field(
+ ..., description="Discriminator - must be 'header'"
+ )
+ text: str = Field(..., min_length=1, description="Header display text")
+ header_size: Literal["SMALL_HEADER", "MEDIUM_HEADER", "LARGE_HEADER"] =
Field(
+ "MEDIUM_HEADER", description="Header text size"
+ )
+ background: Literal["BACKGROUND_TRANSPARENT", "BACKGROUND_WHITE"] = Field(
+ "BACKGROUND_TRANSPARENT", description="Header band background"
+ )
+
+ @field_validator("text")
+ @classmethod
+ def sanitize_text(cls, v: str) -> str:
+ """Sanitize header text to prevent XSS; it renders as plain title
text."""
+ sanitized: str | None = sanitize_user_input(
+ v, "text", max_length=500, allow_empty=True
+ )
+ if not sanitized:
+ raise ValueError("text has no content left after sanitization.")
+ return sanitized
+
+
+class DividerComponentSpec(BaseNewDashboardComponentSpec):
+ """Spec for a new horizontal divider.
+
+ Placed directly on the target grid/tab (not inside a row), same as
+ ``HeaderComponentSpec``. Carries no content — only placement.
+ """
+
+ component_type: Literal["divider"] = Field(
+ ..., description="Discriminator - must be 'divider'"
+ )
+
+
+NewDashboardComponentSpec = Annotated[
+ MarkdownComponentSpec | HeaderComponentSpec | DividerComponentSpec,
+ Field(discriminator="component_type"),
+]
+
+
+class DashboardComponentUpdateSpec(BaseModel):
+ """Partial update for an existing markdown/header/divider component.
+
+ ``id`` and at least one non-null update field are required. Provided
+ fields are merged into the existing component. Fields that only apply to
+ one component type (e.g. ``code`` for markdown, ``text``/``header_size``
+ for header) are rejected when used against the wrong component type.
+ A component's type cannot be changed; remove and re-add instead.
+ """
+
+ model_config = ConfigDict(extra="forbid")
+
+ id: str = Field(..., min_length=1, description="ID of the component to
update")
+ code: str | None = Field(
+ None, min_length=1, description="New markdown source (markdown only)"
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-79: Unsanitized Update Code</b></div>
<div id="fix">
`DashboardComponentUpdateSpec.code` is merged verbatim into the component's
position_json meta (`meta["code"] = spec.code` in `_apply_component_update`),
with no sanitization — the update path reintroduces the same stored-XSS gap as
the add path while `text` updates are sanitized. Sanitize via a shared HTML
allowlist helper. ([CWE-79](https://cwe.mitre.org/data/definitions/79.html))
</div>
</div>
<small><i>Code Review Run #e3ca1e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_markdown.py:
##########
@@ -0,0 +1,1099 @@
+# 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.
+
+"""
+Unit tests for the manage_dashboard_markdown MCP tool.
+
+Follows the pattern from test_manage_native_filters.py:
+- Tests run through the async MCP Client (not direct function calls)
+- auth/editorship are mocked via the directory's autouse mock_auth fixture
+ (tests/unit_tests/mcp_service/dashboard/tool/conftest.py)
+- Patches applied at source locations (superset.daos.dashboard.*, etc.)
+
+Covers:
+- Adding markdown/header/divider components to the default grid and to a tab
+- Updating an existing component (type-specific field rejection)
+- Removing a component (including pruning the wrapper ROW a markdown tile
+ leaves behind)
+- Validation errors: unknown removal ID, update+remove conflict, duplicate
+ update IDs, malformed position_json, missing target tab
+- Header text sanitization
+- Dashboard not found / permission denied
+- "at least one operation" request validation (ToolError at the call boundary)
+"""
+
+from collections.abc import Iterator
+from typing import Any
+from unittest.mock import Mock, patch, PropertyMock
+
+import pytest
+from fastmcp import Client, FastMCP
+from fastmcp.exceptions import ToolError
+from sqlalchemy.exc import SQLAlchemyError
+
+from superset.commands.dashboard.exceptions import DashboardNotFoundError
+from superset.exceptions import SupersetSecurityException
+from superset.utils import json
+
+DAO_GET = "superset.daos.dashboard.DashboardDAO.get_by_id_or_slug"
+
+
[email protected](autouse=True)
+def mock_event_logging() -> Iterator[None]:
+ """Isolate dashboard commits from the event logger's separate audit
commits."""
+ with patch("superset.extensions.event_logger.log_context"):
+ yield
+
+
+def _empty_grid_layout() -> dict[str, Any]:
+ """Build an empty frontend-compatible grid."""
+ return {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"type": "ROOT", "id": "ROOT_ID", "children": ["GRID_ID"]},
+ "GRID_ID": {"type": "GRID", "id": "GRID_ID", "children": []},
+ }
+
+
+def _grid_layout_with_existing_components() -> dict[str, Any]:
+ """Build a grid with each supported component type."""
+ layout = _empty_grid_layout()
+ layout["GRID_ID"]["children"] = [
+ "ROW-existing1",
+ "HEADER-existing1",
+ "DIVIDER-existing1",
+ ]
+ layout["ROW-existing1"] = {
+ "type": "ROW",
+ "id": "ROW-existing1",
+ "children": ["MARKDOWN-existing1"],
+ "meta": {"background": "BACKGROUND_TRANSPARENT"},
+ "parents": ["ROOT_ID", "GRID_ID"],
+ }
+ layout["MARKDOWN-existing1"] = {
+ "type": "MARKDOWN",
+ "id": "MARKDOWN-existing1",
+ "children": [],
+ "meta": {"code": "Hello", "width": 4, "height": 50},
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-existing1"],
+ }
+ layout["HEADER-existing1"] = {
+ "type": "HEADER",
+ "id": "HEADER-existing1",
+ "children": [],
+ "meta": {
+ "text": "Old header",
+ "headerSize": "MEDIUM_HEADER",
+ "background": "BACKGROUND_TRANSPARENT",
+ },
+ "parents": ["ROOT_ID", "GRID_ID"],
+ }
+ layout["DIVIDER-existing1"] = {
+ "type": "DIVIDER",
+ "id": "DIVIDER-existing1",
+ "children": [],
+ "meta": {},
+ "parents": ["ROOT_ID", "GRID_ID"],
+ }
+ return layout
+
+
+def _tabbed_layout() -> dict[str, Any]:
+ """Build a top-level tab layout."""
+ return {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {"type": "ROOT", "id": "ROOT_ID", "children": ["TABS-1"]},
+ "TABS-1": {
+ "type": "TABS",
+ "id": "TABS-1",
+ "children": ["TAB-a", "TAB-b"],
+ "meta": {},
+ },
+ "TAB-a": {
+ "type": "TAB",
+ "id": "TAB-a",
+ "children": [],
+ "meta": {"text": "Overview"},
+ "parents": ["ROOT_ID", "TABS-1"],
+ },
+ "TAB-b": {
+ "type": "TAB",
+ "id": "TAB-b",
+ "children": [],
+ "meta": {"text": "Details"},
+ "parents": ["ROOT_ID", "TABS-1"],
+ },
+ }
+
+
+def _mock_dashboard(
+ id: int = 1,
+ layout: dict[str, Any] | None = None,
+ chart_ids: list[int] | None = None,
+ slug: str | None = None,
+) -> Mock:
+ """Build a dashboard without touching the metadata database."""
+ dashboard = Mock()
+ dashboard.id = id
+ dashboard.dashboard_title = "Test Dashboard"
+ dashboard.slug = slug
+ dashboard.position_json = json.dumps(
+ layout if layout is not None else _empty_grid_layout()
+ )
+ slices = []
+ for chart_id in chart_ids or []:
+ slc = Mock()
+ slc.id = chart_id
+ slices.append(slc)
+ dashboard.slices = slices
+ return dashboard
+
+
+async def _call(mcp_server: FastMCP, request: dict[str, Any]) -> dict[str,
Any]:
+ """Exercise validation and serialization through the MCP boundary."""
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "manage_dashboard_markdown", {"request": request}
+ )
+ return json.loads(result.content[0].text)
+
+
+# ---------------------------------------------------------------------------
+# Add
+# ---------------------------------------------------------------------------
+
+
[email protected]
+async def test_add_markdown_creates_new_row(mcp_server: FastMCP) -> None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [{"component_type": "markdown", "code": "**Hello**"}],
+ },
+ )
+
+ assert data["error"] is None
+ assert len(data["added_component_ids"]) == 1
+ markdown_id = data["added_component_ids"][0]
+ assert markdown_id.startswith("MARKDOWN-")
+
+ saved_layout = json.loads(dashboard.position_json)
+ markdown_node = saved_layout[markdown_id]
+ assert markdown_node["type"] == "MARKDOWN"
+ assert markdown_node["meta"] == {"code": "**Hello**", "width": 4,
"height": 50}
+
+ # Markdown tiles are wrapped in their own new ROW, not placed directly
+ # under GRID_ID.
+ row_key = next(
+ key
+ for key, node in saved_layout.items()
+ if isinstance(node, dict)
+ and node.get("type") == "ROW"
+ and markdown_id in node.get("children", [])
+ )
+ assert row_key in saved_layout["GRID_ID"]["children"]
+
+ summary = next(c for c in data["components"] if c["id"] == markdown_id)
+ assert summary["component_type"] == "markdown"
+
+
[email protected]
+async def test_add_header_placed_directly_under_grid(mcp_server: FastMCP) ->
None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {
+ "component_type": "header",
+ "text": "Sales",
+ "header_size": "LARGE_HEADER",
+ }
+ ],
+ },
+ )
+
+ assert data["error"] is None
+ header_id = data["added_component_ids"][0]
+ assert header_id.startswith("HEADER-")
+
+ saved_layout = json.loads(dashboard.position_json)
+ # HEADER is a full-width band: a direct child of GRID_ID, not wrapped
+ # in a ROW (ROW does not accept HEADER children).
+ assert header_id in saved_layout["GRID_ID"]["children"]
+ assert saved_layout[header_id]["meta"] == {
+ "text": "Sales",
+ "headerSize": "LARGE_HEADER",
+ "background": "BACKGROUND_TRANSPARENT",
+ }
+
+
[email protected]
+async def test_add_divider_placed_directly_under_grid(mcp_server: FastMCP) ->
None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {"dashboard_id": 1, "add": [{"component_type": "divider"}]},
+ )
+
+ assert data["error"] is None
+ divider_id = data["added_component_ids"][0]
+ assert divider_id.startswith("DIVIDER-")
+
+ saved_layout = json.loads(dashboard.position_json)
+ assert divider_id in saved_layout["GRID_ID"]["children"]
+ assert saved_layout[divider_id]["meta"] == {}
+
+
[email protected]
+async def test_add_multiple_components_in_request_order(mcp_server: FastMCP)
-> None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {"component_type": "header", "text": "Section 1"},
+ {"component_type": "markdown", "code": "text"},
+ {"component_type": "divider"},
+ ],
+ },
+ )
+
+ assert data["error"] is None
+ assert len(data["added_component_ids"]) == 3
+ types = [c["component_type"] for c in data["components"]]
+ assert set(types) == {"header", "markdown", "divider"}
+
+
[email protected]
+async def test_add_to_target_tab_by_name(mcp_server: FastMCP) -> None:
+ dashboard = _mock_dashboard(layout=_tabbed_layout())
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {
+ "component_type": "header",
+ "text": "Details header",
+ "target_tab": "Details",
+ }
+ ],
+ },
+ )
+
+ assert data["error"] is None
+ header_id = data["added_component_ids"][0]
+ saved_layout = json.loads(dashboard.position_json)
+ assert header_id in saved_layout["TAB-b"]["children"]
+ assert header_id not in saved_layout["TAB-a"]["children"]
+
+
[email protected]
+async def test_add_target_tab_not_found_lists_available_tabs(
+ mcp_server: FastMCP,
+) -> None:
+ dashboard = _mock_dashboard(layout=_tabbed_layout())
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [
+ {
+ "component_type": "divider",
+ "target_tab": "Nonexistent",
+ }
+ ],
+ },
+ )
+
+ assert "Nonexistent" in data["error"]
+ assert "Overview" in data["error"]
+ assert "Details" in data["error"]
+
+
[email protected]
+async def test_add_target_tab_on_dashboard_without_tabs(mcp_server: FastMCP)
-> None:
+ dashboard = _mock_dashboard()
+
+ with (
+ patch(DAO_GET, return_value=dashboard),
+ patch("superset.extensions.db.session"),
+ ):
+ data = await _call(
+ mcp_server,
+ {
+ "dashboard_id": 1,
+ "add": [{"component_type": "divider", "target_tab":
"Anything"}],
+ },
+ )
+
+ assert "no tabs" in data["error"]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>New tests missing docstrings</b></div>
<div id="fix">
None of the four new tests (`test_add_multiple_components_in_request_order`,
`test_add_to_target_tab_by_name`,
`test_add_target_tab_not_found_lists_available_tabs`,
`test_add_target_tab_on_dashboard_without_tabs`) carries a docstring; BITO rule
12148 requires one per newly added test function documenting the scenario and
expected outcome.
</div>
</div>
<div id="suggestion">
<div id="issue"><b>Locals missing type annotations</b></div>
<div id="fix">
Locals introduced on the changed lines (`dashboard`, `data`, `types`,
`header_id`, `saved_layout`) lack explicit annotations; BITO rules 13153/13347
require them in test files even when inferable (e.g. `dashboard: Mock`,
`saved_layout: dict[str, Any]`).
</div>
</div>
<small><i>Code Review Run #e3ca1e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
