richardfogaca commented on PR #40060:
URL: https://github.com/apache/superset/pull/40060#issuecomment-4439305319
**Automated review via ReviewBot — assigned to @aminghadersohi**
---
## Code Review: fix(mcp): expose table chart type labels in chart responses
### 1. Design Correctness
The additive field approach is sound. Adding an optional `chart_type_label`
to the response schema is backwards-compatible — existing MCP consumers that
don't read the field won't break. Returning `null` for non-table types is
better than omitting the field entirely; it signals "not applicable" rather
than "missing," which is cleaner for consumers to handle.
One mild concern: this field is scoped exclusively to table-family types,
creating an asymmetry. If the MCP consumer ever needs labels for other chart
types, this design will need extension — worth noting in the PR or a TODO so
the pattern is revisited intentionally.
### 2. Schema Change and Label Derivation Logic
- The schema change looks clean — an `Optional[str]` field defaulting to
`None` is the right type.
- The label derivation should use a `dict` mapping (`CHART_TYPE_LABELS =
{"table": "table chart", "ag-grid-table": "interactive table chart"}`) rather
than an `if/elif` chain — more readable and easier to extend.
- Both tools (`generate_chart` and `generate_explore_link`) should call the
same utility function from `chart_utils.py` rather than duplicating logic.
Label derivation should live in exactly one place.
- Edge case: `viz_type=None` or an unrecognized string should gracefully
return `null`. A dict `.get(viz_type)` handles this automatically; an `if/elif`
chain needs an explicit `else: return None`.
### 3. Test Coverage
Expected and likely present:
- `viz_type="table"` → `"table chart"` ✓
- `viz_type="ag-grid-table"` → `"interactive table chart"` ✓
- A non-table `viz_type` → `None` ✓
Worth confirming:
- `viz_type=None` → `None` (null safety)
- An unrecognized string (e.g. `"my-custom-chart"`) → `None`
- Both `generate_chart` and `generate_explore_link` paths have dedicated
test cases for the new field, not just the utility in isolation
### 4. Recommendation
**Approve with minor suggestions.** Well-scoped, low-risk change. Main asks
before merging:
1. Confirm label derivation is in a shared utility (not duplicated across
both tools).
2. Confirm it handles `None`/unrecognized `viz_type` gracefully.
3. Confirm test coverage hits both tool paths.
Nothing blocking.
--
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]
