codeant-ai-for-open-source[bot] commented on code in PR #42658:
URL: https://github.com/apache/superset/pull/42658#discussion_r3726565427
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -570,10 +571,48 @@ def map_table_config(config: TableChartConfig) ->
Dict[str, Any]:
form_data["row_limit"] = config.row_limit
add_color_scheme(form_data, config.color_scheme)
+ if config.column_config is not None:
+ form_data["column_config"] = {
+ label: column.model_dump(by_alias=True, exclude_none=True)
+ for label, column in config.column_config.items()
+ }
Review Comment:
**Suggestion:** `TableColumnConfig` permits nullable formatting fields, but
`exclude_none=True` removes an explicitly supplied `null` before merging.
Consequently, a request that sets `columnWidth`, `d3NumberFormat`, or
`d3TimeFormat` to `null` is indistinguishable from omission and the existing UI
value is preserved instead of cleared. Preserve explicit nulls or provide a
separate clear operation/serialization path. [state lifecycle]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP users cannot clear existing column formatting.
- ⚠️ Table previews retain obsolete D3 formatting settings.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b3b188facd4c4822842a819668c1011b&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=b3b188facd4c4822842a819668c1011b&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/chart_utils.py
**Line:** 574:578
**Comment:**
*State Lifecycle: `TableColumnConfig` permits nullable formatting
fields, but `exclude_none=True` removes an explicitly supplied `null` before
merging. Consequently, a request that sets `columnWidth`, `d3NumberFormat`, or
`d3TimeFormat` to `null` is indistinguishable from omission and the existing UI
value is preserved instead of cleared. Preserve explicit nulls or provide a
separate clear operation/serialization path.
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%2F42658&comment_hash=e8d9e2063a281d0ebc1e03df4f90ec5ecdedc3974c7d394897de4dc03dac27b6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42658&comment_hash=e8d9e2063a281d0ebc1e03df4f90ec5ecdedc3974c7d394897de4dc03dac27b6&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -117,6 +132,7 @@ def _build_update_payload(
parsed_config, dataset_id=effective_dataset_id
)
new_form_data.pop("_mcp_warnings", None)
+ merge_table_column_config(_get_existing_form_data(chart),
new_form_data)
Review Comment:
**Suggestion:** The table-column merge runs for every chart configuration,
even when `parsed_config` is an XY, pie, or other non-table chart. Updating a
table into another visualization therefore persists the old table
`column_config` in the new chart parameters, allowing stale table formatting to
reappear if the chart later returns to a table. Restrict this merge to table
configurations or remove table-only state when changing visualization type.
[api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Visualization changes retain stale table formatting state.
- ⚠️ Returning to tables can apply obsolete column settings.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=01e7c0aeef8346369fd9b47c580bc97a&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=01e7c0aeef8346369fd9b47c580bc97a&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/update_chart.py
**Line:** 135:135
**Comment:**
*Api Mismatch: The table-column merge runs for every chart
configuration, even when `parsed_config` is an XY, pie, or other non-table
chart. Updating a table into another visualization therefore persists the old
table `column_config` in the new chart parameters, allowing stale table
formatting to reappear if the chart later returns to a table. Restrict this
merge to table configurations or remove table-only state when changing
visualization type.
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%2F42658&comment_hash=cb5ac09978b6d22d210d98cdde454270af0815945806457a43db35bf414fdb69&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42658&comment_hash=cb5ac09978b6d22d210d98cdde454270af0815945806457a43db35bf414fdb69&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/update_chart_preview.py:
##########
@@ -186,6 +187,8 @@ def update_chart_preview( # noqa: C901
old_adhoc_filters = previous_form_data.get("adhoc_filters")
if old_adhoc_filters:
new_form_data["adhoc_filters"] = old_adhoc_filters
+ if previous_form_data:
+ merge_table_column_config(previous_form_data, new_form_data)
Review Comment:
**Suggestion:** The preview path imports `column_config` from arbitrary
cached form data without checking the cached chart type or whether the new
configuration is a table. Reusing a preview key while switching to an XY or
another visualization therefore carries table-only formatting and stale column
settings into the new preview. Apply this preservation only when both the
cached and requested configurations are table charts, and clear it on
chart-type transitions. [stale reference]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Preview iterations retain table-only configuration state.
- ⚠️ Switching visualization types can restore obsolete formatting.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dd947f49158d47f2b7c4de308e9c09e8&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=dd947f49158d47f2b7c4de308e9c09e8&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/update_chart_preview.py
**Line:** 190:191
**Comment:**
*Stale Reference: The preview path imports `column_config` from
arbitrary cached form data without checking the cached chart type or whether
the new configuration is a table. Reusing a preview key while switching to an
XY or another visualization therefore carries table-only formatting and stale
column settings into the new preview. Apply this preservation only when both
the cached and requested configurations are table charts, and clear it on
chart-type transitions.
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%2F42658&comment_hash=0add34e136592de62b566303945949861a02eedbf9dcdb94c14a42685c807e44&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42658&comment_hash=0add34e136592de62b566303945949861a02eedbf9dcdb94c14a42685c807e44&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]
