codeant-ai-for-open-source[bot] commented on code in PR #40958:
URL: https://github.com/apache/superset/pull/40958#discussion_r3493610692


##########
superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:
##########
@@ -0,0 +1,491 @@
+# 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.
+
+"""
+MCP tool: remove_chart_from_dashboard
+
+This tool removes a chart from an existing dashboard. It is the inverse of
+add_chart_to_existing_dashboard: it deletes the chart's CHART component(s)
+from position_json (pruning ROW/COLUMN containers that become empty),
+removes the chart from the dashboard's slices relationship, and cleans
+stale references to the chart from json_metadata (expanded_slices,
+timed_refresh_immune_slices, filter_scopes, default_filters).
+"""
+
+import logging
+from typing import Any, Dict
+
+from fastmcp import Context
+from sqlalchemy.exc import SQLAlchemyError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.commands.exceptions import CommandException
+from superset.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+    DashboardInfo,
+    RemoveChartFromDashboardRequest,
+    RemoveChartFromDashboardResponse,
+    serialize_chart_summary,
+)
+from superset.mcp_service.privacy import user_can_view_data_model_metadata
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+# Container types that should be deleted once they have no children left.
+# TAB/TABS/GRID/ROOT containers are intentionally kept even when empty —
+# deleting a TAB would silently change the dashboard's visible structure.
+_PRUNABLE_TYPES = ("ROW", "COLUMN")
+
+
+def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]:
+    """Return all layout keys of CHART components referencing *chart_id*.
+
+    A chart can legitimately appear more than once in a layout (e.g. under
+    multiple tabs), so all occurrences are returned.
+    """
+    # Accept both int and string chartId — position_json is 
user/frontend-authored
+    # and imported or hand-edited layouts may store chartId as a string.
+    return [
+        key
+        for key, node in layout.items()
+        if isinstance(node, dict)
+        and node.get("type") == "CHART"
+        and (node.get("meta") or {}).get("chartId") in (chart_id, 
str(chart_id))
+    ]
+
+
+def _find_parent_key(layout: Dict[str, Any], component_key: str) -> str | None:
+    """Find the component whose children list contains *component_key*.
+
+    The reverse lookup scans children lists instead of trusting the
+    ``parents`` metadata on the node, which can be stale in hand-edited or
+    programmatically generated layouts.
+    """
+    for key, node in layout.items():
+        if not isinstance(node, dict):
+            continue
+        children = node.get("children")
+        if isinstance(children, list) and component_key in children:
+            return key
+    return None
+
+
+def _remove_component_and_prune(
+    layout: Dict[str, Any], component_key: str
+) -> list[str]:
+    """Remove *component_key* from the layout and prune empty containers.
+
+    Walks up the parent chain deleting ROW/COLUMN containers that become
+    empty as a result of the removal, so no orphaned wrapper nodes are left
+    behind. Returns the list of removed layout keys.
+    """
+    removed: list[str] = []
+    parent_key = _find_parent_key(layout, component_key)
+
+    layout.pop(component_key, None)
+    removed.append(component_key)
+
+    child_key = component_key
+    while parent_key is not None:
+        parent = layout.get(parent_key)
+        if not isinstance(parent, dict):
+            break
+        children = parent.get("children")
+        if isinstance(children, list):
+            parent["children"] = [c for c in children if c != child_key]
+        if parent.get("type") in _PRUNABLE_TYPES and not 
parent.get("children"):
+            grandparent_key = _find_parent_key(layout, parent_key)
+            layout.pop(parent_key, None)
+            removed.append(parent_key)
+            child_key = parent_key
+            parent_key = grandparent_key
+        else:
+            break
+
+    return removed
+
+
+def _remove_chart_from_layout(layout: Dict[str, Any], chart_id: int) -> 
list[str]:
+    """Remove every CHART component for *chart_id* from the layout.
+
+    Returns all removed layout keys (charts plus pruned containers).
+    """
+    removed: list[str] = []
+    for chart_key in _find_chart_keys(layout, chart_id):
+        # The chart key may already be gone if it shared a pruned container.
+        if chart_key in layout:
+            removed.extend(_remove_component_and_prune(layout, chart_key))
+    return removed
+
+
+def _remove_id_from_list(values: Any, chart_id: int) -> tuple[Any, bool]:
+    """Return (new_list, changed) with *chart_id* removed from a list of IDs.
+
+    Handles both int and str representations since json_metadata is
+    user/frontend-authored and not strictly typed.
+    """
+    if not isinstance(values, list):
+        return values, False
+    filtered = [v for v in values if v != chart_id and v != str(chart_id)]
+    return filtered, len(filtered) != len(values)
+
+
+def _clean_json_metadata(metadata: Dict[str, Any], chart_id: int) -> bool:
+    """Remove stale references to *chart_id* from a json_metadata dict.
+
+    Cleans ``expanded_slices`` (dict keyed by chart ID), ``filter_scopes``
+    (dict keyed by filter chart ID, with per-column ``immune`` ID lists),
+    ``timed_refresh_immune_slices`` (list of chart IDs), and
+    ``default_filters`` (a JSON-encoded string whose keys are chart IDs).
+    Mutates *metadata* in place and returns True when anything changed.
+    """
+    changed = False
+    chart_key = str(chart_id)
+
+    expanded_slices = metadata.get("expanded_slices")
+    if isinstance(expanded_slices, dict) and chart_key in expanded_slices:
+        del expanded_slices[chart_key]
+        changed = True
+
+    immune_slices, immune_changed = _remove_id_from_list(
+        metadata.get("timed_refresh_immune_slices"), chart_id
+    )
+    if immune_changed:
+        metadata["timed_refresh_immune_slices"] = immune_slices
+        changed = True
+
+    filter_scopes = metadata.get("filter_scopes")
+    if isinstance(filter_scopes, dict):
+        if chart_key in filter_scopes:
+            del filter_scopes[chart_key]
+            changed = True
+        for column_scopes in filter_scopes.values():
+            if not isinstance(column_scopes, dict):
+                continue
+            for column_config in column_scopes.values():
+                if not isinstance(column_config, dict):
+                    continue
+                immune, immune_changed = _remove_id_from_list(
+                    column_config.get("immune"), chart_id
+                )
+                if immune_changed:
+                    column_config["immune"] = immune
+                    changed = True
+
+    # default_filters is stored as a JSON-encoded string within json_metadata,
+    # with chart IDs as string keys (mirrors DashboardDAO.set_dash_metadata).
+    default_filters_raw = metadata.get("default_filters")
+    if isinstance(default_filters_raw, str):
+        try:
+            default_filters = json.loads(default_filters_raw)
+            if isinstance(default_filters, dict) and chart_key in 
default_filters:
+                del default_filters[chart_key]
+                metadata["default_filters"] = json.dumps(default_filters)
+                changed = True
+        except (json.JSONDecodeError, TypeError):
+            pass
+    elif isinstance(default_filters_raw, dict) and chart_key in 
default_filters_raw:
+        del default_filters_raw[chart_key]
+        changed = True

Review Comment:
   **Suggestion:** When `default_filters` is already a dict, this branch 
mutates it in place but leaves it as a dict in `json_metadata`; elsewhere 
Superset expects `default_filters` to be a JSON-encoded string and calls 
`json.loads` on it. Persisting a dict here can therefore trigger runtime 
`TypeError` in downstream readers. Re-serialize and store it back as a string 
after mutation. [type error]
   
   <details>
   <summary><b>Severity Level:</b> Major ⚠️</summary>
   
   ```mdx
   - ❌ Default filter evaluation raises TypeError for mis-typed metadata.
   - ⚠️ Dashboards with dict default_filters remain fragile to failures.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Consider a dashboard whose json_metadata has a top-level 
"default_filters" field stored
   as a JSON object instead of the usual JSON-encoded string; after json.loads,
   metadata["default_filters"] is a dict, which this tool explicitly tolerates
   (superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:191-205).
   
   2. Invoke remove_chart_from_dashboard on that dashboard so that 
_clean_json_metadata at
   lines 149-207 runs; for a dict default_filters_raw, execution enters the 
elif branch at
   line 203, deletes the chart_id key in-place, and leaves 
metadata["default_filters"] as a
   Python dict.
   
   3. The tool then serializes metadata back into 
updated_dashboard.json_metadata via
   json.dumps(metadata) in the json_metadata cleanup path at
   superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:391-407, 
persisting
   "default_filters" as a JSON object instead of a JSON-encoded string 
(contrary to
   DashboardDAO.set_dash_metadata at superset/daos/dashboard.py:261-68, which 
always writes
   md["default_filters"] as json.dumps(applicable_filters)).
   
   4. When Superset later computes default dashboard filters, 
superset/views/utils.py:360-386
   executes json_metadata = json.loads(dashboard.json_metadata) and then 
default_filters =
   json.loads(json_metadata.get("default_filters", "null")); with
   json_metadata["default_filters"] now a dict, json.loads receives a dict and 
raises a
   TypeError, which is not caught by the surrounding
   contextlib.suppress(json.JSONDecodeError), causing the extra filter 
computation to crash
   for that dashboard.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bec9a9eb25ca45b8aa696bea56a2c137&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bec9a9eb25ca45b8aa696bea56a2c137&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <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/remove_chart_from_dashboard.py
   **Line:** 203:205
   **Comment:**
        *Type Error: When `default_filters` is already a dict, this branch 
mutates it in place but leaves it as a dict in `json_metadata`; elsewhere 
Superset expects `default_filters` to be a JSON-encoded string and calls 
`json.loads` on it. Persisting a dict here can therefore trigger runtime 
`TypeError` in downstream readers. Re-serialize and store it back as a string 
after mutation.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=4753dd8dd1ce7d4724fdbf4dd48ac4f9f9248870b82e4d20f927e9285ad8ee5b&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=4753dd8dd1ce7d4724fdbf4dd48ac4f9f9248870b82e4d20f927e9285ad8ee5b&reaction=dislike'>👎</a>



##########
superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:
##########
@@ -0,0 +1,491 @@
+# 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.
+
+"""
+MCP tool: remove_chart_from_dashboard
+
+This tool removes a chart from an existing dashboard. It is the inverse of
+add_chart_to_existing_dashboard: it deletes the chart's CHART component(s)
+from position_json (pruning ROW/COLUMN containers that become empty),
+removes the chart from the dashboard's slices relationship, and cleans
+stale references to the chart from json_metadata (expanded_slices,
+timed_refresh_immune_slices, filter_scopes, default_filters).
+"""
+
+import logging
+from typing import Any, Dict
+
+from fastmcp import Context
+from sqlalchemy.exc import SQLAlchemyError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.commands.exceptions import CommandException
+from superset.extensions import event_logger
+from superset.mcp_service.dashboard.schemas import (
+    DashboardInfo,
+    RemoveChartFromDashboardRequest,
+    RemoveChartFromDashboardResponse,
+    serialize_chart_summary,
+)
+from superset.mcp_service.privacy import user_can_view_data_model_metadata
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+# Container types that should be deleted once they have no children left.
+# TAB/TABS/GRID/ROOT containers are intentionally kept even when empty —
+# deleting a TAB would silently change the dashboard's visible structure.
+_PRUNABLE_TYPES = ("ROW", "COLUMN")
+
+
+def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]:
+    """Return all layout keys of CHART components referencing *chart_id*.
+
+    A chart can legitimately appear more than once in a layout (e.g. under
+    multiple tabs), so all occurrences are returned.
+    """
+    # Accept both int and string chartId — position_json is 
user/frontend-authored
+    # and imported or hand-edited layouts may store chartId as a string.
+    return [
+        key
+        for key, node in layout.items()
+        if isinstance(node, dict)
+        and node.get("type") == "CHART"
+        and (node.get("meta") or {}).get("chartId") in (chart_id, 
str(chart_id))
+    ]
+
+
+def _find_parent_key(layout: Dict[str, Any], component_key: str) -> str | None:
+    """Find the component whose children list contains *component_key*.
+
+    The reverse lookup scans children lists instead of trusting the
+    ``parents`` metadata on the node, which can be stale in hand-edited or
+    programmatically generated layouts.
+    """
+    for key, node in layout.items():
+        if not isinstance(node, dict):
+            continue
+        children = node.get("children")
+        if isinstance(children, list) and component_key in children:
+            return key
+    return None
+
+
+def _remove_component_and_prune(
+    layout: Dict[str, Any], component_key: str
+) -> list[str]:
+    """Remove *component_key* from the layout and prune empty containers.
+
+    Walks up the parent chain deleting ROW/COLUMN containers that become
+    empty as a result of the removal, so no orphaned wrapper nodes are left
+    behind. Returns the list of removed layout keys.
+    """
+    removed: list[str] = []
+    parent_key = _find_parent_key(layout, component_key)
+
+    layout.pop(component_key, None)
+    removed.append(component_key)
+
+    child_key = component_key
+    while parent_key is not None:
+        parent = layout.get(parent_key)
+        if not isinstance(parent, dict):
+            break
+        children = parent.get("children")
+        if isinstance(children, list):
+            parent["children"] = [c for c in children if c != child_key]
+        if parent.get("type") in _PRUNABLE_TYPES and not 
parent.get("children"):
+            grandparent_key = _find_parent_key(layout, parent_key)
+            layout.pop(parent_key, None)
+            removed.append(parent_key)
+            child_key = parent_key
+            parent_key = grandparent_key
+        else:
+            break
+
+    return removed
+
+
+def _remove_chart_from_layout(layout: Dict[str, Any], chart_id: int) -> 
list[str]:
+    """Remove every CHART component for *chart_id* from the layout.
+
+    Returns all removed layout keys (charts plus pruned containers).
+    """
+    removed: list[str] = []
+    for chart_key in _find_chart_keys(layout, chart_id):
+        # The chart key may already be gone if it shared a pruned container.
+        if chart_key in layout:
+            removed.extend(_remove_component_and_prune(layout, chart_key))
+    return removed
+
+
+def _remove_id_from_list(values: Any, chart_id: int) -> tuple[Any, bool]:
+    """Return (new_list, changed) with *chart_id* removed from a list of IDs.
+
+    Handles both int and str representations since json_metadata is
+    user/frontend-authored and not strictly typed.
+    """
+    if not isinstance(values, list):
+        return values, False
+    filtered = [v for v in values if v != chart_id and v != str(chart_id)]
+    return filtered, len(filtered) != len(values)
+
+
+def _clean_json_metadata(metadata: Dict[str, Any], chart_id: int) -> bool:
+    """Remove stale references to *chart_id* from a json_metadata dict.
+
+    Cleans ``expanded_slices`` (dict keyed by chart ID), ``filter_scopes``
+    (dict keyed by filter chart ID, with per-column ``immune`` ID lists),
+    ``timed_refresh_immune_slices`` (list of chart IDs), and
+    ``default_filters`` (a JSON-encoded string whose keys are chart IDs).
+    Mutates *metadata* in place and returns True when anything changed.
+    """
+    changed = False
+    chart_key = str(chart_id)
+
+    expanded_slices = metadata.get("expanded_slices")
+    if isinstance(expanded_slices, dict) and chart_key in expanded_slices:
+        del expanded_slices[chart_key]
+        changed = True
+
+    immune_slices, immune_changed = _remove_id_from_list(
+        metadata.get("timed_refresh_immune_slices"), chart_id
+    )
+    if immune_changed:
+        metadata["timed_refresh_immune_slices"] = immune_slices
+        changed = True
+
+    filter_scopes = metadata.get("filter_scopes")
+    if isinstance(filter_scopes, dict):
+        if chart_key in filter_scopes:
+            del filter_scopes[chart_key]
+            changed = True
+        for column_scopes in filter_scopes.values():
+            if not isinstance(column_scopes, dict):
+                continue
+            for column_config in column_scopes.values():
+                if not isinstance(column_config, dict):
+                    continue
+                immune, immune_changed = _remove_id_from_list(
+                    column_config.get("immune"), chart_id
+                )
+                if immune_changed:
+                    column_config["immune"] = immune
+                    changed = True
+
+    # default_filters is stored as a JSON-encoded string within json_metadata,
+    # with chart IDs as string keys (mirrors DashboardDAO.set_dash_metadata).
+    default_filters_raw = metadata.get("default_filters")
+    if isinstance(default_filters_raw, str):
+        try:
+            default_filters = json.loads(default_filters_raw)
+            if isinstance(default_filters, dict) and chart_key in 
default_filters:
+                del default_filters[chart_key]
+                metadata["default_filters"] = json.dumps(default_filters)
+                changed = True
+        except (json.JSONDecodeError, TypeError):
+            pass
+    elif isinstance(default_filters_raw, dict) and chart_key in 
default_filters_raw:
+        del default_filters_raw[chart_key]
+        changed = True
+
+    return changed
+
+
+def _find_and_authorize_dashboard(
+    dashboard_id: int,
+) -> tuple[Any, RemoveChartFromDashboardResponse | None]:
+    """Return (dashboard, None) on success or (None, error_response) on 
failure.
+
+    Handles both the not-found case and the ownership check so the main tool
+    function doesn't need two separate branches for these pre-conditions.
+    """
+    from superset import security_manager
+    from superset.daos.dashboard import DashboardDAO
+    from superset.exceptions import SupersetSecurityException
+
+    dashboard = DashboardDAO.find_by_id(dashboard_id)
+    if not dashboard:
+        return None, RemoveChartFromDashboardResponse(
+            dashboard=None,
+            dashboard_url=None,
+            error=(
+                f"Dashboard with ID {dashboard_id} not found."
+                " Use list_dashboards to get valid dashboard IDs."
+            ),
+        )
+
+    try:
+        security_manager.raise_for_ownership(dashboard)
+    except SupersetSecurityException:
+        return None, RemoveChartFromDashboardResponse(
+            dashboard=None,
+            dashboard_url=None,
+            permission_denied=True,
+            error=(
+                f"You don't have permission to edit dashboard "
+                f"'{dashboard.dashboard_title}' (ID: {dashboard_id}). "
+                "Inform the user and do not attempt a workaround without "
+                "their confirmation."
+            ),
+        )
+
+    return dashboard, None
+
+
+@tool(
+    tags=["mutate"],
+    class_permission_name="Dashboard",
+    method_permission_name="write",
+    annotations=ToolAnnotations(
+        title="Remove chart from dashboard",
+        readOnlyHint=False,
+        destructiveHint=True,
+    ),
+)
+def remove_chart_from_dashboard(  # noqa: C901 — complexity is structural 
(layout traversal + multi-step authorization), not accidental
+    request: RemoveChartFromDashboardRequest, ctx: Context
+) -> RemoveChartFromDashboardResponse:
+    """
+    Remove a chart from an existing dashboard.
+
+    Deletes the chart's layout component(s) from the dashboard (all
+    occurrences, including under tabs), prunes rows/columns left empty by
+    the removal, detaches the chart from the dashboard, and cleans stale
+    chart references from dashboard metadata (expanded_slices,
+    timed_refresh_immune_slices, filter_scopes, default_filters). The chart
+    itself is NOT deleted and remains available to other dashboards.
+    """
+    try:
+        from superset import db
+        from superset.commands.dashboard.update import UpdateDashboardCommand
+
+        # Validate dashboard exists and user has edit permission
+        with event_logger.log_context(
+            action="mcp.remove_chart_from_dashboard.validation"
+        ):
+            dashboard, auth_error = 
_find_and_authorize_dashboard(request.dashboard_id)
+            if auth_error is not None:
+                return auth_error
+
+        # Remove the chart from the layout tree
+        with 
event_logger.log_context(action="mcp.remove_chart_from_dashboard.layout"):
+            try:
+                current_layout = json.loads(dashboard.position_json or "{}")
+            except (json.JSONDecodeError, TypeError):
+                current_layout = {}
+            if not isinstance(current_layout, dict):
+                current_layout = {}

Review Comment:
   **Suggestion:** If `position_json` is malformed or non-dict, the code 
silently replaces it with `{}` and still writes that value back during update, 
which can wipe the entire dashboard layout while attempting to remove one 
chart. Treat parse/type failures as an error (or preserve original layout) 
instead of continuing with an empty layout payload. [logic error]
   
   <details>
   <summary><b>Severity Level:</b> Critical 🚨</summary>
   
   ```mdx
   - ❌ Dashboard layout reset to empty during chart removal.
   - ⚠️ MCP remove_chart_from_dashboard can corrupt broken dashboards.
   ```
   </details>
   <details>
   <summary><b>Steps of Reproduction ✅ </b></summary>
   
   ```mdx
   1. Create or identify a dashboard row in the database where 
Dashboard.position_json is
   malformed JSON (for example a truncated string), so that
   json.loads(dashboard.position_json or "{}") would raise json.JSONDecodeError 
when
   executed; this field is read directly in remove_chart_from_dashboard at
   superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:289-293.
   
   2. Call the MCP remove_chart_from_dashboard tool for any chart on that 
dashboard; in the
   layout phase at lines 288-293, the try/except catches json.JSONDecodeError 
or TypeError
   and unconditionally sets current_layout = {}, and the subsequent isinstance 
check keeps
   the empty dict.
   
   3. The tool then proceeds to construct update_data at lines 317-320 with 
"position_json":
   json.dumps(current_layout), so UpdateDashboardCommand
   (superset/commands/dashboard/update.py via call at line 322) persists "{}" 
as the new
   Dashboard.position_json, overwriting the existing (albeit malformed) layout 
structure with
   a completely empty layout.
   
   4. Subsequent dashboard loads that rely on position_json (for example, any 
view using the
   layout loaded in superset/views/utils.py:20-27 or front-end layout 
rendering) will see an
   empty layout with all components removed, causing the dashboard to appear 
blank even
   though only a single chart removal operation was requested.
   ```
   </details>
   
   [![Fix in 
Cursor](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-cursor-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c4928d559e9b426fb7404c80da00869f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
 [![Fix in VSCode 
Claude](https://new-codeant-butcket.s3.us-west-1.amazonaws.com/badges/fix-in-vscode-claude-flat.svg)](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c4928d559e9b426fb7404c80da00869f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
   
   *(Use Cmd/Ctrl + Click for best experience)*
   <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/remove_chart_from_dashboard.py
   **Line:** 288:293
   **Comment:**
        *Logic Error: If `position_json` is malformed or non-dict, the code 
silently replaces it with `{}` and still writes that value back during update, 
which can wipe the entire dashboard layout while attempting to remove one 
chart. Treat parse/type failures as an error (or preserve original layout) 
instead of continuing with an empty layout payload.
   
   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.
   Once fix is implemented, also check other comments on the same PR, and ask 
user if the user wants to fix the rest of the comments as well. if said yes, 
then fetch all the comments validate the correctness and implement a minimal fix
   ```
   </details>
   <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=22df0b10c0b0eb4a2011370ccf559dc77f4711d5cb09be449d29ce46267a4367&reaction=like'>👍</a>
 | <a 
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40958&comment_hash=22df0b10c0b0eb4a2011370ccf559dc77f4711d5cb09be449d29ce46267a4367&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]

Reply via email to