fix(mcp): expose table column formatting - #42658
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #42658 +/- ##
==========================================
- Coverage 65.73% 65.71% -0.02%
==========================================
Files 2843 2843
Lines 162653 162681 +28
Branches 37239 37242 +3
==========================================
- Hits 106916 106913 -3
- Misses 53644 53675 +31
Partials 2093 2093
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #770bbdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| 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() | ||
| } |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ MCP users cannot clear existing column formatting.
- ⚠️ Table previews retain obsolete D3 formatting settings.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| 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) |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Visualization changes retain stale table formatting state.
- ⚠️ Returning to tables can apply obsolete column settings.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| if previous_form_data: | ||
| merge_table_column_config(previous_form_data, new_form_data) |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Preview iterations retain table-only configuration state.
- ⚠️ Switching visualization types can restore obsolete formatting.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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 fix2 participants
SUMMARY
Expose per-column table formatting in MCP chart configurations. Table configs can set
columnWidth,d3NumberFormat, andd3TimeFormat, keyed by result column label, and the generated schema documents concrete D3 format examples.Existing table
column_configvalues are preserved when an update omits the field, while explicit values are validated and serialized without silently dropping unknown settings.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
BLAST RADIUS
Limited to MCP table chart configuration schemas and form-data mapping. It does not change authorization, database models, migrations, or non-MCP chart behavior.
RISK AND ROLLBACK
The main risk is accepting a formatting value that the frontend renders differently than expected. Validation constrains field names and primitive types, and rollback is a normal revert with no data migration.
REVIEW GUIDANCE
Start with
TableColumnConfiginschemas.py, then review the form-data mapping and preservation paths in the two update tools.EVAL EVIDENCE
The deterministic schema and round-trip paths are covered by 189 passing focused MCP unit tests. No model-based evaluation suite was available in this checkout.
COST AND LATENCY DELTA
No prompt, model, routing, or tool-call changes. The added local schema validation and dictionary serialization have negligible request cost and latency impact.
PROMPT / NON-DETERMINISM
No prompt or non-deterministic behavior changed.