codeant-ai-for-open-source[bot] commented on code in PR #37970:
URL: https://github.com/apache/superset/pull/37970#discussion_r2835230093
##########
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:
##########
@@ -40,38 +45,89 @@
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.
+ """
+ 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
+
+
+def _find_tab_insert_target(layout: Dict[str, Any]) -> str | None:
"""
- # Find existing rows
- row_indices = []
- for key in layout.keys():
- if key.startswith("ROW-") and key[4:].isdigit():
- row_indices.append(int(key[4:]))
+ Detect if the dashboard uses tabs and return the first tab's ID.
- # Return next available row index
- return max(row_indices) + 1 if row_indices else 0
+ 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
+ parent_component = layout.get(parent_id, {})
+ parent_parents = parent_component.get("parents", [])
Review Comment:
**Suggestion:** When adding a chart to a dashboard whose layout lacks an
existing parent container (e.g. an empty or malformed `position_json` without
`GRID_ID`), the newly created row/column/chart components are assigned a
`parents` chain that omits `ROOT_ID`, producing a hierarchy inconsistent with
other dashboards and with `_create_dashboard_layout`, which can break frontend
assumptions about layout ancestry. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP add-chart tool on empty-layout dashboards stores inconsistent
parents.
- ⚠️ New components miss ROOT_ID ancestor unlike grid container itself.
- ⚠️ Any code assuming full ROOT_ID ancestry may behave unexpectedly.
```
</details>
```suggestion
parent_component = layout.get(parent_id)
if parent_component is not None:
parent_parents = parent_component.get("parents", [])
else:
# For new layouts where GRID_ID doesn't exist yet, assume ROOT_ID
will be created
if parent_id == "GRID_ID":
parent_parents = ["ROOT_ID"]
else:
parent_parents = []
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or select a dashboard whose `position_json` is empty or `NULL` so
that loading
it in Python yields `{}`; this is the layout read at
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:274`
via
`current_layout = json.loads(dashboard.position_json or "{}")`.
2. From an MCP client (e.g., Claude Code) call the
`add_chart_to_existing_dashboard` tool
(function `add_chart_to_existing_dashboard` in
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py`)
with that
dashboard's ID and a valid `chart_id`, so execution enters the layout block
at lines
271–291.
3. Inside the layout block, with `current_layout == {}`,
`_find_tab_insert_target(current_layout)` returns `None` (no `GRID_ID`), so
`parent_id` is
set to `"GRID_ID"` at around line 283, and
`_add_chart_to_layout(current_layout,
new_chart, request.chart_id, row_key, parent_id)` is invoked at line 286.
4. In `_add_chart_to_layout` (same file, lines ~118–160), `parent_component =
layout.get(parent_id, {})` returns `{}` because `"GRID_ID"` is not yet in
the layout;
consequently `parent_parents = parent_component.get("parents", [])` yields
`[]`, so
`row_parents` becomes `["GRID_ID"]` (line 126) and the new `ROW`, `COLUMN`,
and `CHART`
components are created with `parents` chains that do not include
`"ROOT_ID"`, while
`_ensure_layout_structure` later creates `GRID_ID` with parents
`["ROOT_ID"]`. The
resulting stored layout for such dashboards has new components whose
`parents` chains are
inconsistent with the rest of the hierarchy (ROOT_ID is never included in
those chains).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
**Line:** 124:125
**Comment:**
*Logic Error: When adding a chart to a dashboard whose layout lacks an
existing parent container (e.g. an empty or malformed `position_json` without
`GRID_ID`), the newly created row/column/chart components are assigned a
`parents` chain that omits `ROOT_ID`, producing a hierarchy inconsistent with
other dashboards and with `_create_dashboard_layout`, which can break frontend
assumptions about layout ancestry.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37970&comment_hash=fd419f25e296a250f425813f71e26162d9a16d216c788733f29192096bff99a6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37970&comment_hash=fd419f25e296a250f425813f71e26162d9a16d216c788733f29192096bff99a6&reaction=dislike'>👎</a>
--
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]
