codeant-ai-for-open-source[bot] commented on code in PR #36933:
URL: https://github.com/apache/superset/pull/36933#discussion_r2907999433
##########
superset/mcp_service/chart/chart_utils.py:
##########
@@ -39,6 +40,117 @@
from superset.mcp_service.utils.url_utils import get_superset_base_url
from superset.utils import json
+# =============================================================================
+# Visualization Type Constants
+# =============================================================================
+
+# Time series viz types that require a datetime column on the x-axis
+TIMESERIES_VIZ_TYPES = {
+ "echarts_timeseries",
+ "echarts_timeseries_line",
+ "echarts_timeseries_bar",
+ "echarts_timeseries_area",
+ "echarts_timeseries_scatter",
+ "echarts_timeseries_smooth",
+ "echarts_timeseries_step",
+ "mixed_timeseries",
Review Comment:
**Suggestion:** The timeseries visualization type set includes a
non-existent value `"echarts_timeseries_area"` but omits the real area viz type
`"echarts_area"`, so any area chart using `viz_type="echarts_area"` will not be
recognized as a timeseries by `validate_timeseries_config`, causing missing
datetime validation and allowing misconfigured charts that can later fail at
query/render time. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Area charts skip datetime validation in MCP chart utilities.
- ❌ Misconfigured area charts may fail when executing Superset queries.
- ⚠️ Inconsistent validation behavior across different timeseries chart
types.
```
</details>
```suggestion
"echarts_area",
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `superset/mcp_service/chart/chart_utils.py` and inspect the constant
`TIMESERIES_VIZ_TYPES` at lines 48–59, which includes
`"echarts_timeseries_*"` types but
not `"echarts_area"`.
2. In the same file, inspect `validate_timeseries_config` at lines 114–151,
which
immediately returns `None` (skipping validation) when `viz_type not in
TIMESERIES_VIZ_TYPES`.
3. Further down in the same module, inspect `map_xy_config` (in
`chart_utils.py`, after
the helper functions) and note that it maps `XYChartConfig.kind == "area"` to
`viz_type="echarts_area"` in the generated `form_data`.
4. When any caller (e.g., the MCP tools mentioned in the module docstring)
generates an
area chart via `XYChartConfig(kind="area")`, and then passes
`viz_type="echarts_area"`
plus `form_data` into `validate_timeseries_config`, the initial membership
check fails
because `"echarts_area"` is not in `TIMESERIES_VIZ_TYPES`, so no datetime
validation runs;
this allows misconfigured area charts without a proper datetime x‑axis to
pass validation
and later fail at query/render time inside Superset.
```
</details>
<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:** 56:56
**Comment:**
*Logic Error: The timeseries visualization type set includes a
non-existent value `"echarts_timeseries_area"` but omits the real area viz type
`"echarts_area"`, so any area chart using `viz_type="echarts_area"` will not be
recognized as a timeseries by `validate_timeseries_config`, causing missing
datetime validation and allowing misconfigured charts that can later fail at
query/render time.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36933&comment_hash=b686961363e0fae02cd5594e9f95bd043e91e837008b30fc208e769a7fbf460c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36933&comment_hash=b686961363e0fae02cd5594e9f95bd043e91e837008b30fc208e769a7fbf460c&reaction=dislike'>👎</a>
##########
superset/config.py:
##########
@@ -730,7 +731,7 @@ class D3TimeFormat(TypedDict, total=False):
# Enable sharing charts with embedding
# @lifecycle: stable
# @category: runtime_config
- "EMBEDDABLE_CHARTS": True,
+ "EMBEDDABLE_CHARTS": False,
Review Comment:
**Suggestion:** The `EMBEDDABLE_CHARTS` feature flag is defaulted to
`False`, which contradicts the documented/default behavior described in the PR
(where embeddable charts should be enabled by default), so the embed UI and
related functionality will remain hidden unless users manually override the
config, effectively breaking the advertised feature out of the box. [logic
error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ "Embed code" button missing in Explore Share menu.
- ⚠️ MCP tool can't generate embeddable charts via guest token.
```
</details>
```suggestion
"EMBEDDABLE_CHARTS": True,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure Superset following the PR docs: in `superset_config.py` set
`FEATURE_FLAGS =
{"EMBEDDED_SUPERSET": True}` but do NOT define `EMBEDDABLE_CHARTS` (relying
on the
documented default of True).
2. Start the Superset webserver so it loads `superset/config.py`, including
`DEFAULT_FEATURE_FLAGS` where line `734 "EMBEDDABLE_CHARTS": False,` defines
the default
value for this flag.
3. Since `FEATURE_FLAGS` (defined later in `superset/config.py`) is empty by
default and
no `SUPERSET_FEATURE_EMBEDDABLE_CHARTS` env var is set, the effective
feature flags dict
is built using `DEFAULT_FEATURE_FLAGS` with `"EMBEDDABLE_CHARTS": False`, so
`is_feature_enabled("EMBEDDABLE_CHARTS")` will evaluate to False across the
app.
4. Access a chart in Explore or a Dashboard, open the Share menu, and
observe that the
"Embed code" option described in the PR (embeddable charts UI) does not
appear because the
`EMBEDDABLE_CHARTS` flag is disabled by default despite the documentation
stating it
should be enabled.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 734:734
**Comment:**
*Logic Error: The `EMBEDDABLE_CHARTS` feature flag is defaulted to
`False`, which contradicts the documented/default behavior described in the PR
(where embeddable charts should be enabled by default), so the embed UI and
related functionality will remain hidden unless users manually override the
config, effectively breaking the advertised feature out of the box.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36933&comment_hash=fda5e6e5f01ff479f8723030b7a2d06ca7ba5e5a23d44b37029ff7288cc71b60&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F36933&comment_hash=fda5e6e5f01ff479f8723030b7a2d06ca7ba5e5a23d44b37029ff7288cc71b60&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]
