fix(mcp): address reviewer comments — local import rationale, x-optional corrections, cardinality suggestions

- Remove redundant local imports from BigNumberChartPlugin.post_map_validate()
  now that BigNumberChartConfig and is_column_truly_temporal are at top level
- Add explanatory comments on the two remaining local get_registry imports in
  chart_utils.py and dataset_validator.py (circular import prevention)
- Fix schema_validator.py and generate_chart.py docstring: XY 'x' field is
  optional (defaults to dataset primary datetime column), not required
- Propagate cardinality suggestions alongside warnings in XYChartPlugin
- Clarify app.py instructions: chart_type_display_name is null for viz_types
  outside the 7 generate_chart-supported types

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

superset/mcp_service/app.py

@@ -223,13 +223,11 @@ Time grain for temporal x-axis (time_grain parameter):
 
 Chart Types in Existing Charts (viewable via list_charts/get_chart_info):
 Each chart returned by list_charts / get_chart_info includes a
-chart_type_display_name field with a human-readable name. Use that name
-when referring to chart types — do NOT expose raw viz_type identifiers.
-Common display names: Line Chart, Bar Chart, Area Chart, Scatter Plot,
-Pie Chart, Table, Interactive Table, Pivot Table, Big Number,
-Big Number with Trendline, Mixed Timeseries Chart, Custom Template Chart,
-Funnel Chart, Gauge Chart, Heatmap, Sankey Chart, Sunburst, Treemap,
-Word Cloud, World Map, Box Plot, Bubble Chart.
+chart_type_display_name field with a human-readable name when available.
+This field is populated only for the 7 chart types supported by generate_chart
+(xy, pie, table, pivot_table, big_number, mixed_timeseries, handlebars).
+For all other viz_types (Funnel, Gauge, Heatmap, etc.) it will be null —
+use the raw viz_type field instead when referring to those chart types.
 
 Query Examples:
 - List all tables:

superset/mcp_service/chart/chart_utils.py

@@ -325,6 +325,9 @@ def map_config_to_form_data(
     temporal check) are now owned by each plugin's post_map_validate() method
     rather than being baked into this dispatcher.
     """
+    # Local import: plugins call map_*_config from their to_form_data() methods,
+    # so chart_utils is loaded before plugins finish registering. A top-level
+    # import of registry here would trigger plugin loading mid-import = cycle.
     from superset.mcp_service.chart.registry import get_registry
 
     chart_type = getattr(config, "chart_type", None)

superset/mcp_service/chart/plugins/big_number.py

@@ -21,8 +21,9 @@ from __future__ import annotations
 
 from typing import Any
 
+from superset.mcp_service.chart.chart_utils import is_column_truly_temporal
 from superset.mcp_service.chart.plugin import BaseChartPlugin
-from superset.mcp_service.chart.schemas import ColumnRef
+from superset.mcp_service.chart.schemas import BigNumberChartConfig, ColumnRef
 from superset.mcp_service.common.error_schemas import ChartGenerationError
 
 
@@ -144,15 +145,11 @@ class BigNumberChartPlugin(BaseChartPlugin):
         chart_utils.py as a special case. Moving it here keeps the dispatcher
         clean and makes the constraint explicit and discoverable.
         """
-        from superset.mcp_service.chart.schemas import BigNumberChartConfig
-
         if not isinstance(config, BigNumberChartConfig):
             return None
         if not (config.show_trendline and config.temporal_column):
             return None
 
-        from superset.mcp_service.chart.chart_utils import is_column_truly_temporal
-
         if not is_column_truly_temporal(config.temporal_column, dataset_id):
             return ChartGenerationError(
                 error_type="non_temporal_trendline_column",

superset/mcp_service/chart/plugins/xy.py

@@ -179,6 +179,7 @@ class XYChartPlugin(BaseChartPlugin):
                 )
                 if not _ok and card_info:
                     warnings.extend(card_info.get("warnings", []))
+                    warnings.extend(card_info.get("suggestions", []))
         except Exception as exc:
             logger.warning("XY cardinality validation failed: %s", exc)
 

superset/mcp_service/chart/tool/generate_chart.py

@@ -108,7 +108,7 @@ async def generate_chart(  # noqa: C901
     other fields in your configuration:
 
     - Use chart_type='xy' for charts with x and y axes (line, bar, area, scatter)
-      Required fields: x, y
+      Required fields: y (x is optional — defaults to dataset's primary datetime column)
 
     - Use chart_type='table' for tabular visualizations
       Required fields: columns

superset/mcp_service/chart/validation/dataset_validator.py

@@ -377,6 +377,9 @@ class DatasetValidator:
         if not dataset_context:
             return config
 
+        # Local import: plugins call DatasetValidator helpers from normalize_column_refs().
+        # A top-level import of registry in dataset_validator would make loading this
+        # module implicitly trigger plugin registration, creating a circular dependency.
         from superset.mcp_service.chart.registry import get_registry
 
         chart_type = getattr(config, "chart_type", None)

superset/mcp_service/chart/validation/schema_validator.py

@@ -199,7 +199,7 @@ class SchemaValidator:
             "details": "The XY chart configuration is missing required "
             "fields or has invalid structure",
             "suggestions": [
-                "Ensure 'x' field exists with {'name': 'column_name'}",
+                "Note: 'x' is optional and defaults to the dataset's primary datetime column",
                 "Ensure 'y' is an array: [{'name': 'metric', 'aggregate': 'SUM'}]",
                 "Check that all column names are strings",
                 "Verify aggregate functions are valid: SUM, COUNT, AVG, MIN, MAX",