aminghadersohi commented on code in PR #38277:
URL: https://github.com/apache/superset/pull/38277#discussion_r2927133747
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -749,7 +749,7 @@ def _export_data_as_csv(
chart_id=chart.id,
chart_name=chart.slice_name or f"Chart {chart.id}",
chart_type=chart.viz_type or "unknown",
- columns=[], # Not needed for CSV export
+ columns=columns,
Review Comment:
Good catch — fixed in 4794d00. CSV export now passes `columns=[]` since the
column names are embedded in the CSV content itself. The `DataColumn` metadata
is only meaningful for the JSON format.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -904,7 +904,7 @@ def _create_excel_chart_data(
chart_id=chart.id,
chart_name=chart_name,
chart_type=chart.viz_type or "unknown",
- columns=[],
+ columns=list(data[0].keys()) if data else [],
Review Comment:
Same fix as the CSV path — fixed in 4794d00. Excel export now passes
`columns=[]` since column names are embedded in the Excel file.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -89,19 +89,20 @@ def generate(self) -> ChartPreview | ChartError:
class URLPreviewStrategy(PreviewFormatStrategy):
- """Generate URL-based image preview."""
+ """Generate URL-based preview with explore link."""
def generate(self) -> URLPreview | ChartError:
- # Screenshot-based URL previews are not supported.
- # Users should use the explore_url to view the chart interactively,
- # or use other preview formats like 'ascii', 'table', or 'vega_lite'.
- return ChartError(
- error=(
- "URL-based screenshot previews are not supported. "
- "Use the explore_url to view the chart interactively, "
- "or try formats: 'ascii', 'table', or 'vega_lite'."
- ),
- error_type="UnsupportedFormat",
+ chart = self.chart
+ if not chart.id:
+ return ChartError(
+ error="URL preview not available for transient charts without
an ID",
+ error_type="UnsupportedFormat",
+ )
+ explore_url = f"{get_superset_base_url()}/explore/?slice_id={chart.id}"
+ return URLPreview(
+ preview_url=explore_url,
+ width=self.request.width or 800,
+ height=self.request.height or 600,
)
Review Comment:
This is by design for this PR — the URL strategy requires a saved `chart.id`
to construct the explore URL. Before this PR, the URL strategy would always
error out. This PR makes it work for saved charts. Supporting transient/unsaved
charts would need a different strategy (e.g. ASCII or table preview), which
could be a follow-up.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -2037,7 +2038,10 @@ def __init__(self, form_data: Dict[str, Any]):
elif isinstance(content, TablePreview):
result.format = "table"
result.table_data = content.table_data
- # Base64 preview support removed
+ elif isinstance(content, VegaLitePreview):
+ result.format = "vega_lite"
+ elif isinstance(content, URLPreview):
+ result.format = "url"
Review Comment:
Good catch — fixed in 4794d00. Added `result.width = content.width` and
`result.height = content.height` to the URLPreview backward-compat section.
--
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]
