bito-code-review[bot] commented on code in PR #40328:
URL: https://github.com/apache/superset/pull/40328#discussion_r3280942019
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -689,6 +765,126 @@ def _extract_cross_filters_enabled(json_metadata_str: str
| None) -> bool | None
return None
+def _parse_position_json(
+ position_json_str: str | None,
+) -> Dict[str, Any] | None:
+ """Parse position_json into a dict, returning None on any failure."""
+ if not position_json_str:
+ return None
+ try:
+ data = json_loads(position_json_str)
+ except (ValueError, TypeError):
+ return None
+ if not isinstance(data, dict):
+ return None
+ return data
+
+
+def _record_tab(
+ node_id: str,
+ meta: Dict[str, Any],
+ tab_ancestry: tuple[str, ...],
+ tabs_by_id: Dict[str, DashboardTab],
+) -> None:
+ """Register a TAB node into tabs_by_id keyed by component id."""
+ raw_text = meta.get("text")
+ tab_name = raw_text if isinstance(raw_text, str) else None
+ tabs_by_id[node_id] = DashboardTab(
+ id=node_id,
+ name=tab_name,
+ parent_tab_id=tab_ancestry[-1] if tab_ancestry else None,
+ )
+
+
+def _record_chart(
+ meta: Dict[str, Any],
+ tab_ancestry: tuple[str, ...],
+ tabs_by_id: Dict[str, DashboardTab],
+ charts: List[ChartPosition],
+) -> None:
+ """Record a CHART node's position and update enclosing tabs."""
+ raw_chart_id = meta.get("chartId")
+ chart_id = raw_chart_id if isinstance(raw_chart_id, int) else None
+ display_name = meta.get("sliceNameOverride") or meta.get("sliceName")
+ raw_width = meta.get("width")
+ raw_height = meta.get("height")
+ charts.append(
+ ChartPosition(
+ chart_id=chart_id,
+ slice_name=display_name if isinstance(display_name, str) else None,
+ tab_id=tab_ancestry[-1] if tab_ancestry else None,
+ tab_path=[tabs_by_id[t].name or t for t in tab_ancestry if t in
tabs_by_id],
+ width=raw_width if isinstance(raw_width, int) else None,
+ height=raw_height if isinstance(raw_height, int) else None,
+ )
+ )
+ if chart_id is None:
+ return
+ for ancestor_id in tab_ancestry:
+ tab = tabs_by_id.get(ancestor_id)
+ if tab is not None and chart_id not in tab.chart_ids:
+ tab.chart_ids.append(chart_id)
+
+
+def _extract_layout_from_position(
+ position_json_str: str | None,
+) -> tuple[List[DashboardTab], List[ChartPosition]]:
+ """Walk position_json and return (tabs, chart_positions).
+
+ Traverses the component tree iteratively starting from ROOT_ID. Tab
+ ancestry is tracked so chart placement and nested tab references stay
+ accurate. Malformed or missing nodes are skipped silently — partial
+ data is more useful than an exception here, since agents call this
+ tool defensively after seeing the omitted_fields hint.
+ """
+ position = _parse_position_json(position_json_str)
+ if not position or "ROOT_ID" not in position:
+ return [], []
+
+ tabs_by_id: Dict[str, DashboardTab] = {}
+ charts: List[ChartPosition] = []
+
+ stack: List[tuple[str, tuple[str, ...]]] = [("ROOT_ID", ())]
+ visited: set[str] = set()
+
+ while stack:
+ node_id, tab_ancestry = stack.pop()
+ if node_id in visited:
+ continue
+ visited.add(node_id)
+
+ node = position.get(node_id)
+ if not isinstance(node, dict):
+ continue
+
+ node_type = node.get("type")
+ raw_meta = node.get("meta")
+ meta: Dict[str, Any] = raw_meta if isinstance(raw_meta, dict) else {}
+ next_ancestry = tab_ancestry
+
+ if node_type == "TAB":
+ _record_tab(node_id, meta, tab_ancestry, tabs_by_id)
+ next_ancestry = tab_ancestry + (node_id,)
+ elif node_type == "CHART":
+ _record_chart(meta, tab_ancestry, tabs_by_id, charts)
+
+ children = node.get("children")
+ if isinstance(children, list):
+ for child_id in reversed(children):
+ if isinstance(child_id, str):
+ stack.append((child_id, next_ancestry))
+
+ tab_order = [
+ node_id
+ for node_id in position
+ if isinstance(position.get(node_id), dict)
+ and position[node_id].get("type") == "TAB"
+ and node_id in tabs_by_id
+ ]
+ tabs = [tabs_by_id[node_id] for node_id in tab_order]
+ return tabs, charts
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for layout extraction</b></div>
<div id="fix">
The new layout extraction functions (_parse_position_json, _record_tab,
_record_chart, _extract_layout_from_position) lack unit tests. Existing test
file at tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py tests
_extract_cross_filters_enabled and _extract_native_filters but not the new
position parsing logic. Per BITO.md rule [6262], tests should verify actual
business logic directly.
</div>
</div>
<small><i>Code Review Run #e8d97d</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/get_dashboard_layout.py:
##########
@@ -0,0 +1,122 @@
+# 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.
+
+"""
+Get dashboard layout FastMCP tool
+
+Companion to get_dashboard_info: returns the parsed dashboard layout
+(tabs and chart positions) extracted from position_json. Use this
+when get_dashboard_info's omitted_fields hint indicates position_json
+was stripped and structured layout data is needed for analysis.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from sqlalchemy.orm import subqueryload
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ dashboard_layout_serializer,
+ DashboardError,
+ DashboardLayout,
+ GetDashboardLayoutRequest,
+)
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="Dashboard",
+ annotations=ToolAnnotations(
+ title="Get dashboard layout",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_dashboard_layout(
+ request: GetDashboardLayoutRequest, ctx: Context
+) -> DashboardLayout | DashboardError:
+ """
+ Get parsed dashboard layout by ID, UUID, or slug.
+
+ Returns the tabs and chart positions extracted from the dashboard's
+ position_json. get_dashboard_info omits position_json to keep responses
+ small; call this tool when you need the structured layout (e.g. to
+ explain which charts live under which tab, or to locate a chart by
+ its parent tab).
+
+ Example usage:
+ ```json
+ {
+ "identifier": 123
+ }
+ ```
+ """
+ await ctx.info("Retrieving dashboard layout: identifier=%s" %
(request.identifier,))
+
+ try:
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+
+ eager_options = [subqueryload(Dashboard.slices)]
+
+ with
event_logger.log_context(action="mcp.get_dashboard_layout.lookup"):
+ core = ModelGetInfoCore(
+ dao_class=DashboardDAO,
+ output_schema=DashboardLayout,
+ error_schema=DashboardError,
+ serializer=dashboard_layout_serializer,
+ supports_slug=True,
+ logger=logger,
+ query_options=eager_options,
+ )
+ result = core.run_tool(request.identifier)
+
+ if isinstance(result, DashboardLayout):
+ await ctx.info(
+ "Dashboard layout retrieved: id=%s, tab_count=%s,
chart_count=%s, "
+ "has_layout=%s"
+ % (
+ result.id,
+ len(result.tabs),
+ len(result.charts),
+ result.has_layout,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Dashboard layout retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as e:
+ await ctx.error(
+ "Dashboard layout retrieval failed: identifier=%s, error=%s, "
+ "error_type=%s" % (request.identifier, str(e), type(e).__name__)
+ )
+ return DashboardError(
+ error=f"Failed to get dashboard layout: {str(e)}",
+ error_type="InternalError",
+ timestamp=datetime.now(timezone.utc),
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new tool</b></div>
<div id="fix">
The new `get_dashboard_layout` tool lacks unit test coverage. All other
similar MCP tools (`get_dashboard_info`, `get_chart_info`, `get_database_info`,
`get_dataset_info`) have corresponding tests in the test suite. Without tests,
bugs in the layout extraction logic or error handling could go undetected.
</div>
</div>
<small><i>Code Review Run #e8d97d</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/get_dashboard_layout.py:
##########
@@ -0,0 +1,122 @@
+# 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.
+
+"""
+Get dashboard layout FastMCP tool
+
+Companion to get_dashboard_info: returns the parsed dashboard layout
+(tabs and chart positions) extracted from position_json. Use this
+when get_dashboard_info's omitted_fields hint indicates position_json
+was stripped and structured layout data is needed for analysis.
+"""
+
+import logging
+from datetime import datetime, timezone
+
+from fastmcp import Context
+from sqlalchemy.orm import subqueryload
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+ dashboard_layout_serializer,
+ DashboardError,
+ DashboardLayout,
+ GetDashboardLayoutRequest,
+)
+from superset.mcp_service.mcp_core import ModelGetInfoCore
+
+logger = logging.getLogger(__name__)
+
+
+@tool(
+ tags=["discovery"],
+ class_permission_name="Dashboard",
+ annotations=ToolAnnotations(
+ title="Get dashboard layout",
+ readOnlyHint=True,
+ destructiveHint=False,
+ ),
+)
+async def get_dashboard_layout(
+ request: GetDashboardLayoutRequest, ctx: Context
+) -> DashboardLayout | DashboardError:
+ """
+ Get parsed dashboard layout by ID, UUID, or slug.
+
+ Returns the tabs and chart positions extracted from the dashboard's
+ position_json. get_dashboard_info omits position_json to keep responses
+ small; call this tool when you need the structured layout (e.g. to
+ explain which charts live under which tab, or to locate a chart by
+ its parent tab).
+
+ Example usage:
+ ```json
+ {
+ "identifier": 123
+ }
+ ```
+ """
+ await ctx.info("Retrieving dashboard layout: identifier=%s" %
(request.identifier,))
+
+ try:
+ from superset.daos.dashboard import DashboardDAO
+ from superset.models.dashboard import Dashboard
+
+ eager_options = [subqueryload(Dashboard.slices)]
+
+ with
event_logger.log_context(action="mcp.get_dashboard_layout.lookup"):
+ core = ModelGetInfoCore(
+ dao_class=DashboardDAO,
+ output_schema=DashboardLayout,
+ error_schema=DashboardError,
+ serializer=dashboard_layout_serializer,
+ supports_slug=True,
+ logger=logger,
+ query_options=eager_options,
+ )
+ result = core.run_tool(request.identifier)
+
+ if isinstance(result, DashboardLayout):
+ await ctx.info(
+ "Dashboard layout retrieved: id=%s, tab_count=%s,
chart_count=%s, "
+ "has_layout=%s"
+ % (
+ result.id,
+ len(result.tabs),
+ len(result.charts),
+ result.has_layout,
+ )
+ )
+ else:
+ await ctx.warning(
+ "Dashboard layout retrieval failed: error_type=%s, error=%s"
+ % (result.error_type, result.error)
+ )
+
+ return result
+
+ except Exception as e:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Avoid catching blind Exception</b></div>
<div id="fix">
Catching bare `Exception` is too broad (BLE001). Specify the exception types
to catch, such as `ValueError`, `KeyError`, or other specific exceptions that
may be raised.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except (ValueError, KeyError, AttributeError) as e:
````
</div>
</details>
</div>
<small><i>Code Review Run #e8d97d</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]
