codeant-ai-for-open-source[bot] commented on code in PR #39922:
URL: https://github.com/apache/superset/pull/39922#discussion_r3501388925
##########
superset/mcp_service/chart/tool/get_chart_info.py:
##########
@@ -170,6 +170,21 @@ def _apply_unsaved_state_override(result: ChartInfo,
form_data_key: str) -> None
# Update viz_type from cached form_data if present
if result.form_data and "viz_type" in result.form_data:
result.viz_type = result.form_data["viz_type"]
+ if result.viz_type:
+ try:
+ from superset.mcp_service.chart.registry import (
+ display_name_for_viz_type,
+ )
+
+ result.chart_type_display_name =
display_name_for_viz_type(
+ result.viz_type
+ )
+ except Exception as exc: # noqa: BLE001
+ logger.debug(
+ "Failed to resolve display name for viz_type=%r:
%s",
+ result.viz_type,
+ exc,
+ )
Review Comment:
**Suggestion:** The display-name enrichment logic is only applied in the
cached-override branch for saved charts, but not in the form_data_key-only
unsaved-chart path, creating inconsistent responses for the same `viz_type`
depending on lookup path. Extract this mapping into a shared helper and apply
it in both unsaved-chart construction paths. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Unsaved charts lack human-readable chart type display name.
⚠️ API responses differ based on identifier versus form_data_key.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The `get_chart_info` tool entrypoint in
`superset/mcp_service/chart/tool/get_chart_info.py:230-361` supports two
unsaved-state
flows: (a) an unsaved-only chart identified by `form_data_key` without
`identifier`, and
(b) a saved chart with an unsaved override where both `identifier` and
`form_data_key` are
provided.
2. For the unsaved-only path, when `request.identifier` is empty and
`request.form_data_key` is set, `get_chart_info()` takes the branch at
`superset/mcp_service/chart/tool/get_chart_info.py:286-304` and calls
`_build_unsaved_chart_info()` at
`superset/mcp_service/chart/tool/get_chart_info.py:58-91`. That function
parses cached
`form_data`, sets `viz_type=form_data.get("viz_type")` in the `ChartInfo`
constructor at
lines 82-89, but does not derive or populate `chart_type_display_name` from
the registry,
so the field remains `None` even for built-in chart types with known display
names.
3. For the saved-chart-with-unsaved-override path, when both `identifier` and
`form_data_key` are provided, `get_chart_info()` first loads a `ChartInfo`
via
`ModelGetInfoCore` (which already populates `chart_type_display_name` in
`serialize_chart_object()` at
`superset/mcp_service/chart/schemas.py:549-619`), and then
calls `_apply_unsaved_state_override()` at
`superset/mcp_service/chart/tool/get_chart_info.py:160-219`. Inside this
helper, the code
at lines 171-187 updates `result.viz_type` from cached `form_data` and
resolves
`result.chart_type_display_name` via `display_name_for_viz_type()` from
`superset.mcp_service.chart.registry`, ensuring the field is correctly set.
4. As a result, two equivalent unsaved chart configurations with the same
`viz_type`
produce different API outputs: calling `get_chart_info` with only
`form_data_key` returns
a `ChartInfo` whose `chart_type_display_name` is always null, while calling
it with both
`identifier` and `form_data_key` returns a `ChartInfo` where
`chart_type_display_name` is
populated. This inconsistency is observable in the JSON responses from
`get_chart_info`
and contradicts the app-level guidance in
`superset/mcp_service/app.py:356-360` that
charts include a human-readable `chart_type_display_name` when available.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=815985f387fb4ae4be2366cccde501b5&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=815985f387fb4ae4be2366cccde501b5&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/tool/get_chart_info.py
**Line:** 171:187
**Comment:**
*Incomplete Implementation: The display-name enrichment logic is only
applied in the cached-override branch for saved charts, but not in the
form_data_key-only unsaved-chart path, creating inconsistent responses for the
same `viz_type` depending on lookup path. Extract this mapping into a shared
helper and apply it in both unsaved-chart construction paths.
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=d1b2ca8b07b763459798cc00c802aeed848e3e2ecf5fd8612dff8f38a4cb0736&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39922&comment_hash=d1b2ca8b07b763459798cc00c802aeed848e3e2ecf5fd8612dff8f38a4cb0736&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]
