codeant-ai-for-open-source[bot] commented on code in PR #40775:
URL: https://github.com/apache/superset/pull/40775#discussion_r3405706039
##########
superset/mcp_service/app.py:
##########
@@ -327,7 +327,15 @@ def get_default_instructions(
- chart_type="xy", kind="scatter": Scatter plot for correlation analysis
- chart_type="big_number": Big Number display (single metric, header only)
- chart_type="big_number", show_trendline=True,
- temporal_column="<date_col>": Big Number with trendline
+ temporal_column="<date_col>", aggregation="sum": Big Number with trendline
+ (aggregation controls how the value is computed from trendline data points;
+ default when omitted is "LAST_VALUE" — most recent point only.
+ Use aggregation="sum" for all-time totals, "mean" for averages, "max"/"min"
for extremes.
+ DIAGNOSIS: if a Big Number with Trendline shows wrong values, check
+ form_data["aggregation"] — missing/LAST_VALUE means the chart shows only
the last data
+ point, not a total. Fix by setting aggregation="sum" in the update_chart
config.
+ IMPORTANT: always include show_trendline=True and temporal_column when
updating a
+ Big Number with Trendline chart, or the chart type will revert to a plain
Big Number.)
Review Comment:
**Suggestion:** The new guidance says to fix incorrect Big
Number-with-trendline totals by "setting `aggregation="sum"` in the
update_chart config", but `update_chart` validates `config` as a full
`ChartConfig` object, so sending only `aggregation` (plus trendline fields)
will fail because required fields like `metric` are missing. Update this
instruction to explicitly require sending a complete Big Number config (at
minimum `chart_type`, `metric`, `show_trendline`, `temporal_column`, and
`aggregation`) to match the actual API contract. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Big Number trendline fixes fail with invalid update_chart payload.
- ⚠️ LLM clients see validation errors, not updated charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the Big Number with Trendline guidance added to
`get_default_instructions()` in
`superset/mcp_service/app.py` (PR diff lines 329–339). The text says: "Fix
by setting
aggregation="sum" in the update_chart config. IMPORTANT: always include
show_trendline=True and temporal_column…".
2. Note that `update_chart` is implemented in
`superset/mcp_service/chart/tool/update_chart.py:83-155` and its `request`
parameter is
typed as `UpdateChartRequest` (imported at lines 44–49).
3. In `superset/mcp_service/chart/schemas.py:1978-1985`,
`UpdateChartRequest` declares
`config: ChartConfig | None`, and `ChartConfig` is a discriminated union
defined at
`schemas.py:1748-1762` over several concrete config models including
`BigNumberChartConfig`.
4. `BigNumberChartConfig` at `schemas.py:1328-1420` requires both
`chart_type:
Literal["big_number"]` and `metric: ColumnRef = Field(..., ...)` (see lines
1328–1341 and
1341–1348). If an LLM client follows the instructions literally and calls
the MCP
`update_chart` tool with a payload like:
- `request = {"identifier": 123, "config": {"chart_type": "big_number",
"show_trendline": true, "temporal_column": "order_date", "aggregation":
"sum"}}`
omitting `metric` because the instructions only mention `aggregation`,
`show_trendline`, and `temporal_column`,
then Pydantic validation of `UpdateChartRequest` will attempt to
instantiate a
`BigNumberChartConfig` and fail with a validation error (`metric` field
required). This
happens before `_build_update_payload()` at `update_chart.py:98-152` is
invoked, so the
chart is not updated even though the instructions imply that simply
"setting
aggregation='sum' in the update_chart config" is sufficient.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=70291d5339b141b7ae3a8d6ab9751121&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=70291d5339b141b7ae3a8d6ab9751121&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/app.py
**Line:** 334:338
**Comment:**
*Api Mismatch: The new guidance says to fix incorrect Big
Number-with-trendline totals by "setting `aggregation="sum"` in the
update_chart config", but `update_chart` validates `config` as a full
`ChartConfig` object, so sending only `aggregation` (plus trendline fields)
will fail because required fields like `metric` are missing. Update this
instruction to explicitly require sending a complete Big Number config (at
minimum `chart_type`, `metric`, `show_trendline`, `temporal_column`, and
`aggregation`) to match the actual API contract.
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%2F40775&comment_hash=b65b1f927f560ef2b30c07615b5ba41b1ad594e360d4d7b3620a5e82986b9d78&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40775&comment_hash=b65b1f927f560ef2b30c07615b5ba41b1ad594e360d4d7b3620a5e82986b9d78&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]
