codeant-ai-for-open-source[bot] commented on code in PR #43483: URL: https://github.com/apache/superset/pull/43483#discussion_r3846287592
########## superset/mcp_service/chart/resources/chart_viewer/src/components/DataTable.tsx: ########## @@ -0,0 +1,233 @@ +/** + * 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. + */ +import { useEffect, useMemo, useState, type JSX } from 'react'; +import type { ChartData, DataColumn } from '../types'; +import { formatByColumn, stripUntrustedMarkers, toNumber } from '../format'; + +type SortDir = 'asc' | 'desc'; + +/** Selectable page sizes. Superset returns up to ~1000 rows per result. */ +export const PAGE_SIZE_OPTIONS = [25, 50, 100, 250] as const; + +export const DEFAULT_PAGE_SIZE = 25; + +/** Rows for one page, plus the bookkeeping the footer needs to describe it. */ +export interface PageSlice<T> { + rows: T[]; + /** Zero-based index of the page actually shown (clamped into range). */ + page: number; + pageCount: number; + /** One-based row numbers of the visible window, for "xโy of z". */ + from: number; + to: number; + total: number; +} + +/** + * Clamp a requested page into range and slice it out. Pure so the paging + * arithmetic โ the part that silently drops rows when it is wrong โ is + * testable without a DOM. + */ +export function paginate<T>( + rows: T[], + page: number, + pageSize: number, +): PageSlice<T> { + const total = rows.length; + const size = Math.max(1, pageSize); + const pageCount = Math.max(1, Math.ceil(total / size)); + const current = Math.min(Math.max(0, page), pageCount - 1); + const start = current * size; + const slice = rows.slice(start, start + size); + return { + rows: slice, + page: current, + pageCount, + from: total === 0 ? 0 : start + 1, + to: start + slice.length, + total, + }; +} + +/** A dense, sortable, zebra-striped, paginated table with sticky headers. */ +export function DataTable({ + data, + initialPageSize = DEFAULT_PAGE_SIZE, +}: { + data: ChartData; + initialPageSize?: number; +}): JSX.Element { + const columns = data.columns; + const [sortCol, setSortCol] = useState<string | null>(null); + const [sortDir, setSortDir] = useState<SortDir>('asc'); + const [pageSize, setPageSize] = useState(initialPageSize); + const [page, setPage] = useState(0); + + const sorted = useMemo(() => { + const rows = [...(data.data ?? [])]; + if (!sortCol) return rows; + const col = columns.find((c) => c.name === sortCol); + const numeric = col?.data_type === 'numeric'; + rows.sort((a, b) => { + const av = a[sortCol]; + const bv = b[sortCol]; + let cmp: number; + if (numeric) { + cmp = (toNumber(av) ?? -Infinity) - (toNumber(bv) ?? -Infinity); + } else { + cmp = String(av ?? '').localeCompare(String(bv ?? '')); + } + return sortDir === 'asc' ? cmp : -cmp; + }); + return rows; + }, [data.data, sortCol, sortDir, columns]); + + // A re-query, a re-sort or a bigger page all invalidate the current offset; + // land the reader back at the top rather than on an arbitrary window. + useEffect(() => { + setPage(0); + }, [data.data, sortCol, sortDir, pageSize]); + + const slice = useMemo( + () => paginate(sorted, page, pageSize), + [sorted, page, pageSize], + ); + const multiPage = slice.total > PAGE_SIZE_OPTIONS[0]; Review Comment: **Suggestion:** `multiPage` is determined against the smallest available page size rather than the selected `pageSize`. When a user selects 50, 100, or 250 rows and the result contains between 26 and that selected size, the table displays pagination controls even though `slice.pageCount` is one and there is no next page. Compare the total against the active page size instead. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx - โ ๏ธ Table shows unnecessary pagination controls. - โ ๏ธ Single-page results display misleading page navigation. - โ ๏ธ Affects results with 26โ250 rows after larger-page selection. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4ffd40f6a8fd4e8d811461b4e40d1fce&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4ffd40f6a8fd4e8d811461b4e40d1fce&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/resources/chart_viewer/src/components/DataTable.tsx **Line:** 111:111 **Comment:** *Incorrect Condition Logic: `multiPage` is determined against the smallest available page size rather than the selected `pageSize`. When a user selects 50, 100, or 250 rows and the result contains between 26 and that selected size, the table displays pagination controls even though `slice.pageCount` is one and there is no next page. Compare the total against the active page size instead. 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%2F43483&comment_hash=bb94816f1087acf90fca66efafc24ba3022e5c0566d30a9e4b1199b7457dd19b&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=bb94816f1087acf90fca66efafc24ba3022e5c0566d30a9e4b1199b7457dd19b&reaction=dislike'>๐</a> ########## superset/mcp_service/chart/tool/render_chart.py: ########## @@ -0,0 +1,442 @@ +# 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: render_chart (MCP Apps interactive chart widget) + +``render_chart`` returns a chart's data together with a ``_meta.ui.resourceUri`` +descriptor pointing at the ``ui://superset/chart-viewer/v4-<digest>`` UI resource. MCP +Apps hosts (Claude, ChatGPT, VS Code Copilot, Cursor, Goose, ...) fetch that +resource and render the chart-viewer widget in a sandboxed iframe, turning the +tool result into a real interactive visualization instead of a prose summary. + +``render_chart_requery`` is the app-visible companion the widget calls back for +drill-down, brush-to-zoom and filtering. It is marked ``visibility: ["app"]`` so +compliant hosts keep it out of the model's tool list. + +Both tools are thin wrappers over :func:`get_chart_data_core` โ the shared, +already-authorized data path โ so all data access continues to flow through the +same RBAC/RLS-enforcing query pipeline as ``get_chart_data``. +""" + +import logging +from typing import Any + +from fastmcp import Context +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.chart.constants import CHART_VIEWER_URI +from superset.mcp_service.chart.schemas import ( + DashboardCell, + DashboardRender, + RenderDashboardRequest, + ChartData, + ChartError, + GetChartDataRequest, + RenderChartRequeryRequest, + RenderChartRequest, +) +from superset.mcp_service.chart.tool.get_chart_data import get_chart_data_core +from superset.mcp_service.utils.url_utils import get_superset_base_url + +logger = logging.getLogger(__name__) + +# Tool-descriptor _meta for the MCP Apps extension. Note: the iframe CSP is +# declared on the ``ui://`` resource itself (see chart_viewer.py), not here โ the +# MCP Apps spec ignores tool-level ``ui.csp``. We only link the resource and +# declare visibility here. +_RENDER_CHART_UI_META: dict[str, Any] = { + "ui": { + "resourceUri": CHART_VIEWER_URI, + # Both the model (to decide when to render) and the app may call it. + "visibility": ["model", "app"], + } +} +_REQUERY_UI_META: dict[str, Any] = { + "ui": { + "resourceUri": CHART_VIEWER_URI, + # App-visibility is host routing metadata, NOT an authorization boundary: + # render_chart_requery independently runs the same Chart/read RBAC + RLS + # path, so a model that calls it directly gains no extra entitlement. + "visibility": ["app"], + } +} + + +# Design tokens the chart-viewer widget consumes. Deliberately a small, +# explicit allow-list: only presentational values, no URLs/secrets, and bounded +# in size so the widget payload stays small. +_THEME_TOKEN_KEYS: tuple[str, ...] = ( + "colorPrimary", + "colorLink", + "colorError", + "colorWarning", + "colorSuccess", + "colorInfo", + "fontFamily", +) + + +def _instance_theme_tokens() -> dict[str, Any] | None: + """Return the deployment's antd design tokens for the widget. + + Customers configure Superset theming precisely so their visualizations look + consistent; without this the widget would render with hardcoded colors and + drift from the rest of the product. Only an allow-listed subset of + presentational tokens is forwarded. + """ + try: + from flask import current_app + + theme = current_app.config.get("THEME_DEFAULT") or {} + tokens = theme.get("token") or {} + except Exception: # pragma: no cover - theming is best-effort decoration + return None + if not isinstance(tokens, dict): + return None + selected = { + key: tokens[key] + for key in _THEME_TOKEN_KEYS + if isinstance(tokens.get(key), str) + } + return selected or None + + +def _build_explore_url(chart_id: int | None) -> str | None: + """Best-effort absolute Explore deep link for the chart-viewer widget's + "Open in Superset" affordance. + + Built from the resolved numeric ``chart_id`` (from the query result) so it + works even when the caller passed a UUID. Returns ``None`` when there is no + saved chart id (e.g. unsaved charts, ``chart_id == 0``) or no base URL. + """ + if not chart_id: + return None + try: + base_url = get_superset_base_url().rstrip("/") + except Exception: # pragma: no cover - defensive; base URL is best-effort + return None + if not base_url: + return None + return f"{base_url}/explore/?slice_id={chart_id}" + + +async def _render_chart_impl( + request: RenderChartRequest, ctx: Context +) -> ChartData | ChartError: + """Undecorated body of ``render_chart`` (see the tool for docs). Kept + separate so it can be unit-tested without the auth decorator.""" + await ctx.info("Rendering chart: identifier=%s" % (request.identifier,)) + + # Delegate to the shared, authorized data path. Reuse of the core keeps a + # single query/RBAC/RLS pipeline; render_chart adds only presentation. + data_request = GetChartDataRequest( + identifier=request.identifier, + limit=request.limit, + extra_form_data=request.extra_form_data, + use_cache=request.use_cache, + force_refresh=request.force_refresh, + cache_timeout=request.cache_timeout, + format="json", + ) + with event_logger.log_context(action="mcp.render_chart"): + result = await get_chart_data_core(data_request, ctx) + + if isinstance(result, ChartData): + result.explore_url = _build_explore_url(result.chart_id) + result.theme = _instance_theme_tokens() + return result + + +@tool( + tags=["data"], + class_permission_name="Chart", + annotations=ToolAnnotations( + title="Render chart", + readOnlyHint=True, + destructiveHint=False, + ), + meta=_RENDER_CHART_UI_META, +) +async def render_chart( + request: RenderChartRequest, ctx: Context +) -> ChartData | ChartError: + """Render a saved chart as an interactive visualization in the chat. + + Use this instead of ``get_chart_data`` when the user wants to *see* a chart, + not just read its numbers. On MCP Apps-capable hosts the result renders as a + real, interactive chart (line/bar/area/table/big-number) inline in the + conversation; on other hosts the same structured data and text summary are + returned so the model can describe it. + + Pass a chart ``identifier`` (numeric ID or UUID). Optionally narrow the data + with ``extra_form_data`` filters or a row ``limit``. + """ + return await _render_chart_impl(request, ctx) + + +def _resolve_filter( + request: RenderChartRequeryRequest, +) -> tuple[Any | None, Any | None]: + """Resolve the drill filter column/value from either the flat + ``filter_col``/``filter_val`` fields or the widget's ``filter={col,val}`` + object form. The object form takes precedence when present.""" + col = request.filter_col + val = request.filter_val + if isinstance(request.filter, dict): + col = request.filter.get("col", col) + # Support both {"val": ...} and {"value": ...}. + if "val" in request.filter: + val = request.filter["val"] + elif "value" in request.filter: + val = request.filter["value"] + return col, val + + +def _requery_extra_form_data( + request: RenderChartRequeryRequest, +) -> dict[str, Any]: + """Translate widget interactions into a Superset ``extra_form_data`` override + that the shared query path understands.""" + extra: dict[str, Any] = {} + filters: list[dict[str, Any]] = [] + + filter_col, filter_val = _resolve_filter(request) + if filter_col is not None and filter_val is not None: + filters.append({"col": filter_col, "op": "==", "val": filter_val}) + if filters: + extra["filters"] = filters + if request.time_range is not None: + extra["time_range"] = request.time_range + if request.granularity is not None: + # Superset reads the time grain from extra_form_data.time_grain_sqla. + extra["time_grain_sqla"] = request.granularity + return extra + + +async def _render_chart_requery_impl( + request: RenderChartRequeryRequest, ctx: Context +) -> ChartData | ChartError: + """Undecorated body of ``render_chart_requery`` (see the tool for docs).""" + await ctx.info( + "Re-querying chart for widget: identifier=%s, time_range=%s" + % (request.identifier, request.time_range) + ) + + extra_form_data = _requery_extra_form_data(request) + + data_request = GetChartDataRequest( + identifier=request.identifier, + limit=request.limit, + extra_form_data=extra_form_data or None, + use_cache=request.use_cache, + force_refresh=request.force_refresh, + cache_timeout=request.cache_timeout, + format="json", + ) + with event_logger.log_context(action="mcp.render_chart_requery"): + result = await get_chart_data_core(data_request, ctx) + + if isinstance(result, ChartData): + result.explore_url = _build_explore_url(result.chart_id) + result.theme = _instance_theme_tokens() + return result + + +@tool( + tags=["data"], + class_permission_name="Chart", + annotations=ToolAnnotations( + title="Re-query chart (widget drill-down)", + readOnlyHint=True, + destructiveHint=False, + ), + meta=_REQUERY_UI_META, +) +async def render_chart_requery( + request: RenderChartRequeryRequest, ctx: Context +) -> ChartData | ChartError: + """Re-query a chart for the interactive widget (drill-down / zoom / filter). + + Called by the chart-viewer widget when the user clicks a data point, brushes + a time range, or drills by a dimension. Not intended for direct model use. + Runs through the same authorized data path as ``render_chart``. + """ + return await _render_chart_requery_impl(request, ctx) + + +async def _render_dashboard_impl( + request: RenderDashboardRequest, ctx: Context +) -> DashboardRender | ChartError: + """Undecorated body of ``render_dashboard`` (see the tool for docs).""" + from superset.daos.dashboard import DashboardDAO + from superset.mcp_service.dashboard.schemas import ( + DashboardError, + DashboardLayout, + dashboard_layout_serializer, + ) + from superset.mcp_service.mcp_core import ModelGetInfoCore + + await ctx.info("Rendering dashboard: identifier=%s" % (request.identifier,)) + + # Reuse get_dashboard_layout's core rather than re-parsing position_json: + # one parser, one set of tab/position semantics. + with event_logger.log_context(action="mcp.render_dashboard.layout"): + layout = ModelGetInfoCore( + dao_class=DashboardDAO, + output_schema=DashboardLayout, + error_schema=DashboardError, + serializer=dashboard_layout_serializer, + supports_slug=True, + logger=logger, + ).run_tool(request.identifier) Review Comment: **Suggestion:** The dashboard layout lookup does not handle exceptions from `ModelGetInfoCore.run_tool`. Unlike the existing `get_dashboard_layout` tool, which converts lookup and serialization failures into a structured error response, any database or serializer exception here escapes the render tool and causes the entire dashboard render to fail instead of returning a `ChartError` or per-dashboard error. [error handling] <details> <summary><b>Severity Level:</b> Major โ ๏ธ</summary> ```mdx - โ Dashboard widget rendering fails on layout lookup errors. - โ ๏ธ Clients receive no structured `ChartError` response. - โ ๏ธ A single layout/serialization failure prevents dashboard composition. ``` </details> [](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90f5704524ea47058cc8642e3ef184e6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=90f5704524ea47058cc8642e3ef184e6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) <details> <summary><b>Prompt for AI Agent ๐ค </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/tool/render_chart.py **Line:** 299:307 **Comment:** *Error Handling: The dashboard layout lookup does not handle exceptions from `ModelGetInfoCore.run_tool`. Unlike the existing `get_dashboard_layout` tool, which converts lookup and serialization failures into a structured error response, any database or serializer exception here escapes the render tool and causes the entire dashboard render to fail instead of returning a `ChartError` or per-dashboard error. 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%2F43483&comment_hash=1a75641afa3a6e23b82bd68236375c63380f464a0b4c1a05a13703d491dd8ada&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43483&comment_hash=1a75641afa3a6e23b82bd68236375c63380f464a0b4c1a05a13703d491dd8ada&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]
