codeant-ai-for-open-source[bot] commented on code in PR #38409:
URL: https://github.com/apache/superset/pull/38409#discussion_r2885672251
##########
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:
##########
@@ -82,13 +86,25 @@ def _find_tab_insert_target(layout: Dict[str, Any]) -> str
| 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
+ if not tabs_children:
+ continue
+
+ # When a target_tab is specified, try to resolve it by name or ID
+ if 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", "")
+ if target_tab in (tab_id, tab_text):
+ return tab_id
+
+ # Fallback: return the first TAB child
+ 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
Review Comment:
**Suggestion:** The tab selection helper returns a fallback tab from the
first TABS container inside the loop, so if a requested tab name/ID only exists
in a later TABS group, the function never searches those later groups and
incorrectly inserts the chart into the first tab set (or fails to find any TAB
when the first group's children are non-TABs), leading to charts appearing
under the wrong tab or outside any tab container. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MCP add_chart_to_existing_dashboard misplaces charts in wrong tab.
- ⚠️ Automations cannot reliably target specific tabs on complex dashboards.
```
</details>
```suggestion
first_tab_fallback: str | None = None
for child_id in grid.get("children", []):
child = layout.get(child_id)
if not child or child.get("type") != "TABS":
continue
tabs_children = child.get("children", [])
if not tabs_children:
continue
# Record the first TAB child across all TABS containers as fallback
if first_tab_fallback is None:
for tab_id in tabs_children:
tab = layout.get(tab_id)
if tab and tab.get("type") == "TAB":
first_tab_fallback = tab_id
break
# When a target_tab is specified, try to resolve it by name or ID
if 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", "")
if target_tab in (tab_id, tab_text):
return tab_id
return first_tab_fallback
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Superset dashboard with a v2 layout where `GRID_ID` has multiple
`TABS`
children in its `children` array (e.g. two separate tab groups), and ensure
this layout is
persisted in `Dashboard.position_json` for that dashboard
(`superset/models/dashboard.py:426-428` governs DB access).
2. In the Superset MCP environment, call the
`add_chart_to_existing_dashboard` tool
(`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py`,
function
`add_chart_to_existing_dashboard`) with `request.dashboard_id` set to that
dashboard,
`request.chart_id` to an existing chart, and `request.target_tab` equal to
the `meta.text`
(or ID) of a `TAB` component that exists **only** under the second `TABS`
group, not the
first.
3. During the request, the tool parses `dashboard.position_json` into
`current_layout` and
invokes `_find_tab_insert_target(current_layout,
target_tab=request.target_tab)` at lines
303-307 in `add_chart_to_existing_dashboard.py` to resolve where to insert
the new row.
4. `_find_tab_insert_target` (lines 66-108) iterates `GRID_ID["children"]`,
encounters the
first `TABS` container, fails to find `target_tab` among its `TAB` children,
then executes
the fallback block and returns the first `TAB` of that first group, never
examining later
`TABS` containers; `_add_chart_to_layout` and `_ensure_layout_structure`
then insert the
chart row under this wrong tab, so the chart appears under an unexpected tab
instead of
the requested one.
```
</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:** 86:108
**Comment:**
*Logic Error: The tab selection helper returns a fallback tab from the
first TABS container inside the loop, so if a requested tab name/ID only exists
in a later TABS group, the function never searches those later groups and
incorrectly inserts the chart into the first tab set (or fails to find any TAB
when the first group's children are non-TABs), leading to charts appearing
under the wrong tab or outside any tab container.
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%2F38409&comment_hash=1b8f35a3288530ffc3b097171dcbd26620855411030641de7c76bce318fb9c63&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38409&comment_hash=1b8f35a3288530ffc3b097171dcbd26620855411030641de7c76bce318fb9c63&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]
