codeant-ai-for-open-source[bot] commented on code in PR #43262: URL: https://github.com/apache/superset/pull/43262#discussion_r3797385673
########## superset/widgets/api.py: ########## @@ -0,0 +1,219 @@ +# 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. +from __future__ import annotations + +import logging +from typing import Any + +from flask import make_response, request, Response +from flask_appbuilder.api import expose, protect, safe +from flask_babel import gettext as _ + +# Importing registers the built-in widget control sets into the registry. +import superset.widgets.builtin # noqa: F401 pylint: disable=unused-import +from superset.extensions import event_logger +from superset.utils import json +from superset.views.base_api import BaseSupersetApi, statsd_metrics +from superset.widgets.registry import registry +from superset.widgets.schema_tools import get_subtrees, SchemaPathError + +logger = logging.getLogger(__name__) + + +class WidgetControlsRestApi(BaseSupersetApi): + """ + Schema-driven controls for Dashboard V2 widgets (experimental). + + Serves the backend-owned control JSON Schema that drives both the dashboard + Inspector's control panel and read-only MCP progressive disclosure. Widget + data is fetched separately via the v1 chart-data path on the frontend, so + there is no data endpoint here. The resource name ``widgets`` is + a placeholder (see ``WIDGET_FRAMEWORK.md``). + """ + + resource_name = "widgets" + allow_browser_login = True + # Read-only schema serving; reuse the existing Chart permission so no new + # permission is introduced. + class_permission_name = "Chart" + method_permission_name = { + "types": "read", + "control_schema": "read", + } + openapi_spec_tag = "Dashboard Controls (experimental)" + + @expose("/types", methods=("GET",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.types", + log_to_statsd=False, + ) + def types(self) -> Response: + """List registered building-widget control sets. + --- + get: + summary: List widget types that have a schema-driven control panel + responses: + 200: + description: A list of widget types + content: + application/json: + schema: + type: object + properties: + result: + type: array + items: + type: object + 401: + $ref: '#/components/responses/401' + """ + result = [ + {"id": cls.widget_type, "name": cls.name, "description": cls.description} + for cls in registry.list() + ] + return self.response(200, result=result) + + @expose("/type/<widget_type>/control-schema", methods=("GET", "POST")) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: ( + f"{self.__class__.__name__}.control_schema" + ), + log_to_statsd=False, + ) + def control_schema(self, widget_type: str) -> Response: + """Return the control JSON Schema for a widget type (progressive + disclosure). + + ``GET`` returns the base schema (no enrichment) — enough to discover a + type's fields and required props without a CSRF-bearing request. ``POST`` + additionally accepts ``control_values``/``series`` to enrich x-dynamic + fields, and an optional ``paths`` array to return just those subtrees + (a ``{path: schema}`` map) instead of the whole schema. + --- + get: + summary: Get the base control schema for a widget type + parameters: + - in: path + name: widget_type + required: true + schema: + type: string + responses: + 200: + description: Control JSON Schema + content: + application/json: + schema: + type: object + properties: + result: + type: object + 404: + $ref: '#/components/responses/404' + post: + summary: Get the enriched control schema (or requested subtrees) + parameters: + - in: path + name: widget_type + required: true + schema: + type: string + requestBody: + required: false + content: + application/json: + schema: + type: object + properties: + control_values: + type: object + series: + type: array + items: + type: string + paths: + type: array + items: + type: string + description: >- + When given, return a `{path: schema}` map of just these + drill-in subtrees instead of the whole schema. + responses: + 200: + description: Control JSON Schema, or a `{path: schema}` map + content: + application/json: + schema: + type: object + properties: + result: + type: object + warning: + type: string + 400: + $ref: '#/components/responses/400' + 404: + $ref: '#/components/responses/404' + """ + widget = registry.get(widget_type) + if widget is None: + return self.response_404() + # GET carries no body (and no CSRF); tolerate a missing/non-JSON body so + # the base schema is returned for a plain read. + body = request.get_json(silent=True) or {} + control_values = body.get("control_values") + series = body.get("series") + + warning: str | None = None + try: + schema = widget.get_control_schema(control_values, series) + except Exception: # pylint: disable=broad-except + # Enrichment can fail (e.g. malformed dynamic values); showing the + # base form is better than a hard error — mirror the Semantic Layer. + warning = str( + _( + "Could not enrich the controls for this widget; showing the " + "default form. See the server logs for details." + ) + ) + logger.exception( + "Error enriching control schema for widget type %s", widget_type + ) + schema = widget.get_control_schema(None, None) + + # `paths` narrows the response to just those drill-in subtrees; without + # it the whole (enriched) schema is returned. + if paths := body.get("paths"): + try: + result: dict[str, Any] = get_subtrees(schema, paths) Review Comment: **Suggestion:** The documented contract requires `paths` to be an array of strings, but this code accepts any truthy value and passes it directly to `get_subtrees`. A request such as `paths: "dataBinding"` is iterated character-by-character and produces misleading path handling instead of a validation error; a non-object JSON body can also fail earlier at `body.get(...)`. Validate the request body and require `paths` to be a list of strings before processing it. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Malformed schema requests can produce HTTP 500 responses. - ⚠️ MCP or frontend clients receive inconsistent path errors. - ⚠️ API contract documents an array but accepts arbitrary types. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bef83fb5f3de445dbc3cc1de232c0923&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=bef83fb5f3de445dbc3cc1de232c0923&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/widgets/api.py **Line:** 206:208 **Comment:** *Api Mismatch: The documented contract requires `paths` to be an array of strings, but this code accepts any truthy value and passes it directly to `get_subtrees`. A request such as `paths: "dataBinding"` is iterated character-by-character and produces misleading path handling instead of a validation error; a non-object JSON body can also fail earlier at `body.get(...)`. Validate the request body and require `paths` to be a list of strings before processing it. 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%2F43262&comment_hash=1dc35c021f23e2c2ef14176e7ac2d0a040976f3fb246fea2416c40d2e9f288dd&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43262&comment_hash=1dc35c021f23e2c2ef14176e7ac2d0a040976f3fb246fea2416c40d2e9f288dd&reaction=dislike'>👎</a> ########## superset-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx: ########## @@ -0,0 +1,248 @@ +/** + * 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. + */ + +/** + * A widget's control panel, driven by a backend-owned JSON Schema. + * + * The schema is fetched from `/api/v1/widgets`, rendered generically + * with JsonForms, and edits are written straight back to the node's `props` — + * the same object the widget reads and the same object an assistant's + * `updateProps` writes, so a change here and one made in chat are the same edit + * by different routes. Data is discovered via the v1 chart-data path + * (`fetchQueryData`). + * + * For widgets with an `x-dynamic` sub-schema (e.g. balloons' per-series + * `customize`), the panel discovers the widget's distinct series values from the + * query results and posts them back, so the backend can enrich the schema (the + * SIP's x-dynamic pattern; ignored by schemas that don't declare it). + * + * NOTE: must be rendered bare — NOT inside an antd `Form`, which would bind the + * generated `Form.Item`s to its own store and swallow the edits. The Inspector + * renders it bare for exactly this reason (see `PropsEditor`). + */ +import { useEffect, useMemo, useRef, useState } from 'react'; +import { SupersetClient } from '@superset-ui/core'; +import { Loading, Typography } from '@superset-ui/core/components'; +import { JsonForms } from '@jsonforms/react'; +import type { JsonSchema } from '@jsonforms/core'; +import { cellRegistryEntries } from '@great-expectations/jsonforms-antd-renderers'; +import type { dashboard as dashboardApi } from '@apache-superset/core'; +import { + buildUiSchema, + sanitizeSchema, +} from 'src/features/semanticLayers/jsonFormsHelpers'; +import { provider, useDashboardRevision } from 'src/core/dashboard/store'; +import { fetchQueryData } from 'src/core/dashboard/chartData'; +import { schemaControlRenderers } from './schemaControlRenderers'; + +type DataBindingSpec = dashboardApi.DataBindingSpec; +type WidgetProps = Record<string, unknown>; + +/** + * Distinct values of the widget's color dimension, so a schema with an + * `x-dynamic` per-series section can enumerate them. This must match the + * dimension the widget colors by (its `colorDimension`, or the last grouping + * dimension by default — see `BalloonsWidget`), so the customizable series line + * up with the balloons on screen. Empty when the binding can't be queried. + */ +async function loadSeries( + binding: DataBindingSpec, + colorDimension: string, +): Promise<string[]> { + const { rows } = await fetchQueryData(binding); + const seen: string[] = []; + rows.forEach(row => { + const value = String(row[colorDimension] ?? ''); + if (!seen.includes(value)) seen.push(value); + }); + return seen; +} + +async function fetchControlSchema( + widgetType: string, + controlValues: WidgetProps, + series: string[], +): Promise<JsonSchema> { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/widgets/type/${widgetType}/control-schema`, + jsonPayload: { control_values: controlValues, series }, + }); + return (json as { result: JsonSchema }).result; +} + +/** + * SupersetClient rejects a non-2xx response with the raw, unparsed `Response` + * object rather than an `Error`, so a plain `String(e)` yields the useless + * "[object Response]". Pull the actual `{message}`/`{errors:[...]}` body Superset + * sends back (same shape `chartData.ts` handles). + */ +async function describeError(e: unknown): Promise<string> { + if (typeof Response !== 'undefined' && e instanceof Response) { + try { + const body = await e.clone().json(); + const detail = + body?.message ?? + (Array.isArray(body?.errors) + ? body.errors + .map((err: { message?: string }) => err.message) + .join('; ') + : undefined); + return detail + ? `${e.status} ${e.statusText}: ${detail}` + : `${e.status} ${e.statusText}`; + } catch { + return `${e.status} ${e.statusText}`; + } + } + return e instanceof Error ? e.message : String(e); +} + +/** True once a binding has enough to run a grouped query. */ +function canQuery( + binding: DataBindingSpec | undefined, +): binding is DataBindingSpec { + return Boolean( + binding?.datasetId && binding.metrics?.length && binding.dimensions?.length, + ); +} + +export default function SchemaControlPanel({ nodeId }: { nodeId: string }) { + useDashboardRevision(); + const node = provider.getNode(nodeId); + const widgetType = node?.type ?? ''; + const props = useMemo<WidgetProps>( + () => (node?.props as WidgetProps) ?? {}, + [node?.props], + ); + const binding = props.dataBinding as DataBindingSpec | undefined; + const bindingKey = JSON.stringify(binding ?? null); + // The dimension whose distinct values become the customizable series: the + // explicit `colorDimension` when it's one of the grouping dimensions, else the + // last dimension (mirrors BalloonsWidget's default). + const dimensions = binding?.dimensions ?? []; + const explicitColor = props.colorDimension as string | undefined; + const colorDimension = + explicitColor && dimensions.includes(explicitColor) + ? explicitColor + : dimensions[dimensions.length - 1]; + + const [series, setSeries] = useState<string[]>([]); + const [schema, setSchema] = useState<JsonSchema | null>(null); + const [error, setError] = useState<string | null>(null); + + // Discover series once the binding has a grouping dimension; empty otherwise. + // Only relevant to schemas that declare an x-dynamic field, but harmless for + // the rest (the backend ignores `series` when nothing depends on it). + useEffect(() => { + if (!canQuery(binding) || !colorDimension) { + setSeries([]); + return undefined; + } + let cancelled = false; + loadSeries(binding, colorDimension) + .then(result => !cancelled && setSeries(result)) + .catch(() => !cancelled && setSeries([])); + return () => { + cancelled = true; + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [bindingKey, colorDimension]); + + // (Re)fetch the schema when the widget type or discovered series change. The + // series change is what carries an x-dynamic dependency (e.g. a new grouping + // dimension) through to a re-enriched schema. + const seriesKey = JSON.stringify(series); + useEffect(() => { + if (!widgetType) return undefined; + let cancelled = false; + fetchControlSchema(widgetType, props, series) Review Comment: **Suggestion:** Once a schema request fails, `error` is never cleared on a later successful request, so a transient failure leaves the error message permanently rendered even after the schema has loaded successfully. Clear the error before starting a request and after a successful response. [error handling] <details> <summary><b>Severity Level:</b> Minor 🧹</summary> ```mdx - ⚠️ Inspector retains an obsolete schema error message. - ⚠️ Users receive misleading failure status after recovery. - ⚠️ Later schema loads remain visually associated with failure. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c509a419410041c9a2341f46e8f0294c&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=c509a419410041c9a2341f46e8f0294c&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-frontend/src/pages/DashboardBuilderV2/SchemaControlPanel.tsx **Line:** 174:174 **Comment:** *Error Handling: Once a schema request fails, `error` is never cleared on a later successful request, so a transient failure leaves the error message permanently rendered even after the schema has loaded successfully. Clear the error before starting a request and after a successful response. 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%2F43262&comment_hash=018c1f1d849b704eaf399dd5d8b7984ae1eed9dd4ca24109baa9ae8dee5334b8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43262&comment_hash=018c1f1d849b704eaf399dd5d8b7984ae1eed9dd4ca24109baa9ae8dee5334b8&reaction=dislike'>👎</a> ########## superset/widgets/builtin.py: ########## @@ -0,0 +1,120 @@ +# 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. +"""Built-in Dashboard V2 widget control sets. Importing this registers them.""" + +from __future__ import annotations + +from copy import deepcopy +from typing import Any + +from pydantic import BaseModel + +from superset.widgets.controls import ( + AgGridTableControls, + BalloonsControls, + EchartsControls, + MarkdownControls, + MetricTileControls, +) +from superset.widgets.registry import registry, WidgetControls + + [email protected] +class Markdown(WidgetControls): + widget_type = "markdown" + name = "Markdown" + description = "Rich text authored in Markdown." + controls_class = MarkdownControls + + [email protected] +class Echarts(WidgetControls): + widget_type = "echarts" + name = "ECharts" + description = "A chart from a raw ECharts option with $bind data markers." + controls_class = EchartsControls + + [email protected] +class MetricTile(WidgetControls): + widget_type = "metric-tile" + name = "Metric Tile" + description = "A single live metric value rendered as a big number." + controls_class = MetricTileControls + + [email protected] +class AgGridTable(WidgetControls): + widget_type = "ag-grid-table" + name = "Table" + description = "Query results rendered as an AG Grid table." + controls_class = AgGridTableControls + + [email protected] +class Balloons(WidgetControls): + """ + Explicit/typed chart: renders one balloon per query row, colored and sized + per series. The per-series ``customize`` section is populated dynamically + once a grouping dimension is chosen and the frontend reports the distinct + series values (the SIP's ``x-dynamic`` pattern). + """ + + widget_type = "balloons" + name = "Balloons" + description = "Bouncing colored balls, one per query row (Chart Framework v2 POC)." + controls_class = BalloonsControls + + # Default color per series index. Must match the frontend widget's palette + # so a series' color is stable before the author touches the customize + # control. + PALETTE = ["#e74c3c", "#3498db", "#2ecc71", "#f1c40f", "#9b59b6", "#1abc9c"] + + @classmethod + def enrich_schema( + cls, + schema: dict[str, Any], + parsed: BaseModel | None, + series: list[str], + ) -> None: + # Nested models land in $defs; the x-dynamic field is Customization.series. + defs = schema.get("$defs", {}) + series_prop = defs.get("Customization", {}).get("properties", {}).get("series") + style_def = defs.get("SeriesStyle") + if series_prop is None or style_def is None: + return + # Only populate once a grouping dimension is set and the frontend has + # reported the distinct series values from the query results. + dimensions = None + if parsed is not None: + data_binding = getattr(parsed, "data_binding", None) + dimensions = getattr(data_binding, "dimensions", None) + if not dimensions or not series: + return + # Replace the open-ended map with one inlined, pre-colored style per series. + series_prop.pop("additionalProperties", None) + properties: dict[str, Any] = {} + for index, value in enumerate(series): + style = deepcopy(style_def) + style["properties"]["color"]["default"] = cls.PALETTE[ + index % len(cls.PALETTE) + ] + # Title each group with the series value so the control panel labels + # it by series rather than by the shared model name ("SeriesStyle"). + style["title"] = value + properties[value] = style Review Comment: **Suggestion:** The endpoint accepts caller-supplied `series` values without a count or size limit, and this loop deep-copies the complete `SeriesStyle` schema for every supplied entry. An authenticated caller can submit a very large or duplicate series list and force unbounded schema construction and JSON serialization work, causing excessive CPU and memory use. Bound and deduplicate the series values before enrichment. [performance] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Authenticated callers can trigger excessive schema CPU work. - ⚠️ Large responses consume worker memory and bandwidth. - ❌ Repeated requests can degrade the widget-control API service. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5284150199704e3e9d4136d09fe8378b&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=5284150199704e3e9d4136d09fe8378b&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/widgets/builtin.py **Line:** 111:119 **Comment:** *Performance: The endpoint accepts caller-supplied `series` values without a count or size limit, and this loop deep-copies the complete `SeriesStyle` schema for every supplied entry. An authenticated caller can submit a very large or duplicate series list and force unbounded schema construction and JSON serialization work, causing excessive CPU and memory use. Bound and deduplicate the series values before enrichment. 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%2F43262&comment_hash=15a8975ec770b4be0f147640f0ed70f9de6cdf6faa6383a0ad40e07e8be74f41&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43262&comment_hash=15a8975ec770b4be0f147640f0ed70f9de6cdf6faa6383a0ad40e07e8be74f41&reaction=dislike'>👎</a> ########## superset-frontend/src/core/dashboard/WidgetView.tsx: ########## @@ -202,26 +202,26 @@ const BuildingBlockView = forwardRef<HTMLDivElement, BuildingBlockViewProps>( ref={ref} {...rest} // Where a node is on screen, for the panels that reach into the - // canvas from outside it — the Outline scrolls to the block it just + // canvas from outside it — the Outline scrolls to the widget it just // selected by finding it here. Set after the spread so a parent // renderer cannot displace a node's own identity. data-node-id={nodeId} - // Every block is a thing an author selects, so every block is a + // Every widget is a thing an author selects, so every widget is a // control — announced as one, reachable by Tab, and answering the // keys a control answers. The outline offers the same selection in a - // tree, but a block you can point at and not reach from the keyboard - // is still a block half the people using this cannot select. + // tree, but a widget you can point at and not reach from the keyboard + // is still a widget half the people using this cannot select. // A real `button` is not available: this element carries its own // ref and an injected `style` (see this component's own doc - // comment), and a block's content is interactive in its own right — + // comment), and a widget's content is interactive in its own right — // a chart, a table — which a `button` may not contain. // eslint-disable-next-line jsx-a11y/prefer-tag-over-role role="button" Review Comment: **Suggestion:** The selectable wrapper is exposed as `role="button"` and handles every bubbling Enter/Space keydown, but it contains real interactive controls such as tabs and header `ActionButton`s. Keyboard activation on those child controls bubbles here, where `preventDefault()` runs before the child's default button activation, so pressing Enter or Space can select the widget instead of switching tabs, removing it, or toggling it. Do not apply button keyboard behavior to a wrapper containing interactive descendants, or stop propagation at the child controls and use a non-button selection mechanism. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Keyboard tab controls can fail to switch or remove tabs. - ❌ Header collapse, add-slide, and remove actions can be suppressed. - ⚠️ Widget selection occurs instead of the intended child action. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=72c9c195d89440f486aeea3c8545f71f&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=72c9c195d89440f486aeea3c8545f71f&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-frontend/src/core/dashboard/WidgetView.tsx **Line:** 219:234 **Comment:** *Possible Bug: The selectable wrapper is exposed as `role="button"` and handles every bubbling Enter/Space keydown, but it contains real interactive controls such as tabs and header `ActionButton`s. Keyboard activation on those child controls bubbles here, where `preventDefault()` runs before the child's default button activation, so pressing Enter or Space can select the widget instead of switching tabs, removing it, or toggling it. Do not apply button keyboard behavior to a wrapper containing interactive descendants, or stop propagation at the child controls and use a non-button selection mechanism. 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%2F43262&comment_hash=0f8ad69760db27a36788c8d9f39ffbf13a68ba20c71831b68d890da8c5644112&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43262&comment_hash=0f8ad69760db27a36788c8d9f39ffbf13a68ba20c71831b68d890da8c5644112&reaction=dislike'>👎</a> ########## superset-frontend/src/core/dashboard/DashboardProvider.ts: ########## @@ -315,11 +311,7 @@ class DashboardProvider { this.commit(nodes); } - public moveBuildingBlock( - id: string, - newParentId: string, - newIndex: number, - ): void { + public moveWidget(id: string, newParentId: string, newIndex: number): void { Review Comment: **Suggestion:** Cross-container moves preserve `layout.rowSpan`, even though flow containers interpret that field as a pixel height while the root grid interprets it as a row-track span. Moving a root-grid widget into a flow container therefore applies values such as `rowSpan: 8` as an 8-pixel height, and moving a flowed widget back can produce an unintended grid span. Clear or convert `rowSpan` when changing container types. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Cross-container drag can collapse widgets to tiny heights. - ❌ Returning flowed widgets can create enormous grid spans. - ⚠️ Affects dashboard reparenting through `RootGrid`. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=95a0ea825e28469c95445ad330bb0b2c&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=95a0ea825e28469c95445ad330bb0b2c&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-frontend/src/core/dashboard/DashboardProvider.ts **Line:** 314:314 **Comment:** *Logic Error: Cross-container moves preserve `layout.rowSpan`, even though flow containers interpret that field as a pixel height while the root grid interprets it as a row-track span. Moving a root-grid widget into a flow container therefore applies values such as `rowSpan: 8` as an 8-pixel height, and moving a flowed widget back can produce an unintended grid span. Clear or convert `rowSpan` when changing container types. 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%2F43262&comment_hash=d6b1f798767e5d08f4f8844e5ae7104e1ebabcab0fcdf56fe8dc8cf7cb2cc3c7&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43262&comment_hash=d6b1f798767e5d08f4f8844e5ae7104e1ebabcab0fcdf56fe8dc8cf7cb2cc3c7&reaction=dislike'>👎</a> ########## superset-frontend/src/core/dashboard/chartData.ts: ########## @@ -56,9 +60,22 @@ export async function fetchQueryData( ): Promise<QueryDataResult> { const formData = { datasource: `${binding.datasetId}__table`, - metrics: binding.metrics, - groupby: binding.dimensions ?? [], - adhoc_filters: binding.filters ?? [], + // Drop empty/blank inputs the schema-driven control panel can seed (an + // empty array item — a blank dimension string, or an empty `{}` filter + // object) before they reach `buildQueryContext`, which throws on a + // malformed adhoc filter (e.g. `{}`) rather than ignoring it. + metrics: (binding.metrics ?? []).filter( + metric => metric != null && metric !== '', + ), + groupby: (binding.dimensions ?? []).filter( + dimension => typeof dimension === 'string' && dimension !== '', + ), + adhoc_filters: (binding.filters ?? []).filter( + filter => + filter != null && + typeof filter === 'object' && + Object.keys(filter).length > 0, + ), Review Comment: **Suggestion:** The filter only removes empty objects and accepts any nonempty object, including arrays and malformed objects without `clause` or the required filter fields. `buildQueryContext` passes these to `processFilters`, which can call `sanitizeClause` with an undefined SQL expression and throw before the request is sent. Validate the filter shape or discard invalid entries rather than treating every nonempty object as a valid adhoc filter. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Invalid widget bindings crash query construction. - ⚠️ `SchemaControlPanel.loadSeries()` depends on `fetchQueryData`. - ⚠️ Users receive runtime errors instead of validation messages. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2e8cb6b135b42e28f75bde093e22346&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=d2e8cb6b135b42e28f75bde093e22346&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-frontend/src/core/dashboard/chartData.ts **Line:** 73:78 **Comment:** *Type Error: The filter only removes empty objects and accepts any nonempty object, including arrays and malformed objects without `clause` or the required filter fields. `buildQueryContext` passes these to `processFilters`, which can call `sanitizeClause` with an undefined SQL expression and throw before the request is sent. Validate the filter shape or discard invalid entries rather than treating every nonempty object as a valid adhoc filter. 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%2F43262&comment_hash=f1d63cc602644fe113e9427c87c672231b063099b2abd971767542bbd54e6bbb&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43262&comment_hash=f1d63cc602644fe113e9427c87c672231b063099b2abd971767542bbd54e6bbb&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]
