codeant-ai-for-open-source[bot] commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3482478663
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -102,7 +105,15 @@ class ChartInfo(BaseModel):
id: int | None = Field(None, description="Chart ID")
slice_name: str | None = Field(None, description="Chart name")
- viz_type: str | None = Field(None, description="Visualization type")
+ viz_type: str | None = Field(None, description="Visualization type
(internal ID)")
+ chart_type_display_name: str | None = Field(
+ None,
+ description=(
+ "User-friendly chart type name (e.g. 'Line Chart', 'Pivot Table').
"
+ "Prefer this field when referring to chart types; "
+ "fall back to viz_type when this field is null."
+ ),
+ )
Review Comment:
**Suggestion:** The new `chart_type_display_name` field is added to
`ChartInfo` but it is not included in the default chart column selections used
by `get_chart_info`/`list_charts`, so callers will not receive this field
unless they explicitly request it. Add this field to the default column sets to
avoid shipping a feature that is silently absent in normal responses.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ list_charts responses omit chart_type_display_name field by default.
- ❌ get_chart_info responses omit chart_type_display_name field by default.
- ⚠️ MCP app documentation promises chart_type_display_name in responses.
- ⚠️ LLM agents lose friendly chart type names.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `list_charts` tool without specifying `select_columns`
(default usage
documented in `superset/mcp_service/chart/tool/list_charts.py:85-103`). The
request is
parsed into `ListChartsRequest`, whose `select_columns` field defaults to an
empty list
via `default_factory=list` in `superset/mcp_service/chart/schemas.py:21-27`.
2. In `list_charts`, the `ModelListCore` instance is constructed with
`default_columns=DEFAULT_CHART_COLUMNS` at
`superset/mcp_service/chart/tool/list_charts.py:170-176`, where
`DEFAULT_CHART_COLUMNS` is
defined at `list_charts.py:52-62` and does **not** include
`"chart_type_display_name"`.
3. The list result is serialized with `result.model_dump(mode="json",
context={"select_columns": columns_to_filter})` at `list_charts.py:207-217`,
where
`columns_to_filter` is `result.columns_requested` (derived from
`DEFAULT_CHART_COLUMNS`
when `select_columns` is not provided). Each nested `ChartInfo` uses its
model serializer
`_filter_fields_by_context` at
`superset/mcp_service/chart/schemas.py:179-195`, which
filters fields to only those present in `select_columns`. Since
`"chart_type_display_name"` is not in that list, it is omitted from the JSON
response even
though `serialize_chart_object` populates it at `schemas.py:191-199,195`.
4. The same issue occurs for `get_chart_info`:
`GetChartInfoRequest.select_columns`
defaults to `DEFAULT_GET_CHART_INFO_COLUMNS` via `_parse_select_columns` at
`superset/mcp_service/chart/schemas.py:286-355`, and
`DEFAULT_GET_CHART_INFO_COLUMNS` at
`schemas.py:263-283` also omits `"chart_type_display_name"`.
`get_chart_info` returns
`result.model_dump(..., context={"select_columns": request.select_columns})`
at
`superset/mcp_service/chart/tool/get_chart_info.py:144-147`, so
`ChartInfo._filter_fields_by_context` again removes
`chart_type_display_name`. This
contradicts the MCP app documentation in `superset/mcp_service/app.py:8-14`,
which
explicitly promises that `list_charts` and `get_chart_info` responses
include a
`chart_type_display_name` field.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=53af6c002b904f8ea8a455985ebe0863&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=53af6c002b904f8ea8a455985ebe0863&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/chart/schemas.py
**Line:** 108:116
**Comment:**
*Incomplete Implementation: The new `chart_type_display_name` field is
added to `ChartInfo` but it is not included in the default chart column
selections used by `get_chart_info`/`list_charts`, so callers will not receive
this field unless they explicitly request it. Add this field to the default
column sets to avoid shipping a feature that is silently absent in normal
responses.
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%2F39922&comment_hash=82abdfd9c8b5999e1fd438f17b3a2f86fd971b71b21fa408646dac5e8cf7d0ba&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=82abdfd9c8b5999e1fd438f17b3a2f86fd971b71b21fa408646dac5e8cf7d0ba&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -561,11 +572,27 @@ def serialize_chart_object(chart: ChartLike | None) ->
ChartInfo | None:
# Extract structured filter information
filters_info = extract_filters_from_form_data(chart_form_data)
+ _viz_type = getattr(chart, "viz_type", None)
+ _display_name = None
+ if _viz_type:
+ try:
+ from superset.mcp_service.chart.registry import
display_name_for_viz_type
+ except ImportError:
+ pass
+ else:
+ try:
+ _display_name = display_name_for_viz_type(_viz_type)
+ except Exception as exc: # noqa: BLE001 — third-party plugins may
raise
+ logger.debug(
+ "Failed to resolve display name for viz_type=%r: %s",
_viz_type, exc
+ )
Review Comment:
**Suggestion:** `chart_type_display_name` is computed from the saved chart
`viz_type`, but `get_chart_info` can later override `viz_type` from unsaved
`form_data_key` state without recomputing this display field, which can return
mismatched values (new `viz_type` with old display name). Recompute
`chart_type_display_name` whenever `viz_type` is overridden from unsaved form
data. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ get_chart_info can return inconsistent viz_type and display name.
- ⚠️ LLM agents may misinterpret actual chart visualization type.
- ⚠️ Unsaved Explore edits yield confusing chart type metadata.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A saved chart (Slice) is serialized through `get_chart_info` using
`ModelGetInfoCore`
with `serializer=serialize_chart_object` at
`superset/mcp_service/chart/tool/get_chart_info.py:101-107`. Inside
`serialize_chart_object` at `superset/mcp_service/chart/schemas.py:150-220`,
`_viz_type`
is read from `chart.viz_type` at `schemas.py:575` and `_display_name` is
computed via
`display_name_for_viz_type(_viz_type)` at `schemas.py:578-585`, then both
are stored into
`ChartInfo(viz_type=_viz_type, chart_type_display_name=_display_name)` at
`schemas.py:191-197`.
2. In Explore, the user edits the chart and changes its `viz_type` (chart
type) without
saving, producing cached `form_data` keyed by `form_data_key` as described
in the Explore
flow (`form_data_key` handling in `superset/views/core.py:395-418` and
`superset/explore/api.py:66-68`). The MCP tool `get_chart_info` explicitly
documents using
`form_data_key` from the Explore URL to fetch unsaved state at
`superset/mcp_service/chart/tool/get_chart_info.py:241-247`.
3. The client calls MCP `get_chart_info` with both a chart `identifier` and
`form_data_key`. The tool first loads the saved chart via `ModelGetInfoCore`
(step 1),
then since `request.form_data_key` is set, it calls
`_apply_unsaved_state_override(result,
request.form_data_key)` at `get_chart_info.py:114-124`. Inside
`_apply_unsaved_state_override` at `get_chart_info.py:160-176`, cached form
data is loaded
and, if it contains `"viz_type"`, `result.viz_type` is overridden from
`result.form_data["viz_type"]` at lines 170-172. However, this function does
**not**
recompute or clear `result.chart_type_display_name`; subsequent sanitization
at
`get_chart_info.py:188-203` only revalidates and reassigns `filters` and
`form_data`.
4. Finally, `get_chart_info` returns `result.model_dump(mode="json",
context={"select_columns": request.select_columns})` at
`get_chart_info.py:144-147`. When
callers include `"chart_type_display_name"` in `select_columns` (as
encouraged by the MCP
app docs at `superset/mcp_service/app.py:8-14`), they can receive a
mismatched pair such
as `viz_type="echarts_timeseries"` from unsaved state but
`chart_type_display_name="Table"` preserved from the original saved chart.
This violates
the contract that `chart_type_display_name` reflects the chart's actual type.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=35d771939e6c430c9d47b2a6215df34a&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=35d771939e6c430c9d47b2a6215df34a&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/chart/schemas.py
**Line:** 575:588
**Comment:**
*Logic Error: `chart_type_display_name` is computed from the saved
chart `viz_type`, but `get_chart_info` can later override `viz_type` from
unsaved `form_data_key` state without recomputing this display field, which can
return mismatched values (new `viz_type` with old display name). Recompute
`chart_type_display_name` whenever `viz_type` is overridden from unsaved form
data.
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%2F39922&comment_hash=1bdd815d3cf900a1919767c72357bed022e217c123213c8e3a8ff0075e636272&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=1bdd815d3cf900a1919767c72357bed022e217c123213c8e3a8ff0075e636272&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]
