codeant-ai-for-open-source[bot] commented on code in PR #40399:
URL: https://github.com/apache/superset/pull/40399#discussion_r3467025674
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -461,6 +461,117 @@ async def test_generate_dashboard_creation_failure(
# rollback called by tool + event_logger error handling
assert mock_db_session.rollback.call_count >= 1
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_generate_dashboard_duplicate_slug_returns_actionable_error(
+ self,
+ mock_db_session,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
+ """When the supplied slug collides with an existing dashboard,
+ ``commit()`` raises ``IntegrityError``. The tool catches it,
+ recognises the slug-uniqueness violation, and returns a
+ structured error naming the offending slug so the LLM can
+ propose a different one — instead of the generic
+ "internal error" message used for other DB failures."""
+ from sqlalchemy.exc import IntegrityError
+
+ mock_query = Mock()
+ mock_filter = Mock()
+ mock_query.filter.return_value = mock_filter
+ mock_query.filter_by.return_value = mock_filter
+ mock_filter.order_by.return_value = mock_filter
+ mock_filter.all.return_value = [_mock_chart(id=1)]
+ mock_filter.first.return_value = Mock(
+ id=1,
+ username="admin",
+ first_name="Admin",
+ last_name="User",
+ email="[email protected]",
+ active=True,
+ )
+ mock_db_session.query.return_value = mock_query
+ # Postgres-shaped IntegrityError naming the slug constraint.
+ mock_db_session.commit.side_effect = IntegrityError(
+ statement="INSERT INTO dashboards ...",
+ params={},
+ orig=Exception(
+ "duplicate key value violates unique constraint "
+ '"dashboards_slug_key"\n'
+ "DETAIL: Key (slug)=(my-slug) already exists."
+ ),
+ )
+ mock_dashboard_cls.return_value = _mock_dashboard(id=100)
+
+ request = {
+ "chart_ids": [1],
+ "dashboard_title": "Q4 Review",
+ "slug": "my-slug",
+ }
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("generate_dashboard", {"request":
request})
+
+ err = result.structured_content["error"]
+ assert err is not None
+ assert "my-slug" in err
+ assert "already in use" in err
+ assert "different slug" in err or "Choose a different" in err
+ assert result.structured_content["dashboard"] is None
+ assert mock_db_session.rollback.call_count >= 1
+
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_generate_dashboard_integrity_error_unrelated_to_slug(
+ self,
+ mock_db_session,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
Review Comment:
**Suggestion:** Add parameter type hints to this new async test method so
both inputs and output are fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added async test method has untyped parameters. That matches the
custom rule requiring fully typed new Python code, so the suggestion is valid.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8750bf3294d04066b5e190a761ef043e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=8750bf3294d04066b5e190a761ef043e&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:**
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
**Line:** 528:533
**Comment:**
*Custom Rule: Add parameter type hints to this new async test method so
both inputs and output are fully typed.
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%2F40399&comment_hash=3e25c4a2c9ee57e6a838c3b6b11934985752873e96b6eee78ba7140ca04894d8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=3e25c4a2c9ee57e6a838c3b6b11934985752873e96b6eee78ba7140ca04894d8&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -575,6 +686,160 @@ async def
test_generate_dashboard_empty_string_title_preserved(
created = mock_dashboard_cls.return_value
assert created.dashboard_title == ""
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_generate_dashboard_position_json_override(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
+ """An explicit ``position_json`` replaces the auto-generated layout
+ in full — the tool serializes the caller's dict verbatim into the
+ dashboard's ``position_json`` column rather than calling the
+ layout-builder helper."""
+ from superset.utils import json
+
+ charts = [_mock_chart(id=1, slice_name="Sales")]
+ mock_dashboard = _mock_dashboard(id=70, title="Custom Layout
Dashboard")
+ _setup_generate_dashboard_mocks(
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ charts,
+ mock_dashboard,
+ )
+
+ custom_layout = {
+ "ROOT_ID": {"type": "ROOT", "children": ["GRID_ID"]},
+ "GRID_ID": {"type": "GRID", "children": ["ROW-custom"]},
+ "ROW-custom": {
+ "type": "ROW",
+ "children": ["CHART-1"],
+ "meta": {"background": "BACKGROUND_TRANSPARENT"},
+ },
+ "CHART-1": {
+ "type": "CHART",
+ "children": [],
+ "meta": {"chartId": 1, "width": 12, "height": 100},
+ },
+ }
+ request = {
+ "chart_ids": [1],
+ "dashboard_title": "Custom Layout Dashboard",
+ "position_json": custom_layout,
+ }
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("generate_dashboard", {"request":
request})
+
+ assert result.structured_content["error"] is None
+ created = mock_dashboard_cls.return_value
+ # position_json on the model is a JSON string; round-trip it
+ # and verify the caller's layout — not the auto-generated 2-col
+ # grid — was written.
+ stored = json.loads(created.position_json)
+ assert stored == custom_layout
+ # The auto-generated layout's HEADER/ROW ids wouldn't match
+ # `ROW-custom`; this sanity-check guards against regressions
+ # where the override silently merges with the default.
+ assert "ROW-custom" in stored
+
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_generate_dashboard_json_metadata_overrides_shallow_merged(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
+ """``json_metadata_overrides`` is shallow-merged on top of the
+ defaults: caller-supplied keys win, defaults for keys the caller
+ didn't touch survive."""
+ from superset.utils import json
+
+ charts = [_mock_chart(id=1, slice_name="Sales")]
+ mock_dashboard = _mock_dashboard(id=71, title="Themed Dashboard")
+ _setup_generate_dashboard_mocks(
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ charts,
+ mock_dashboard,
+ )
+
+ overrides = {
+ "label_colors": {"Electronics": "#4C78A8"},
+ "cross_filters_enabled": True,
+ }
+ request = {
+ "chart_ids": [1],
+ "dashboard_title": "Themed Dashboard",
+ "json_metadata_overrides": overrides,
+ }
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("generate_dashboard", {"request":
request})
+
+ assert result.structured_content["error"] is None
+ created = mock_dashboard_cls.return_value
+ merged = json.loads(created.json_metadata)
+ # Caller overrides land verbatim
+ assert merged["label_colors"] == {"Electronics": "#4C78A8"}
+ assert merged["cross_filters_enabled"] is True
+ # Defaults for keys the caller did NOT supply must still be
+ # present — this is what makes it a *shallow merge* rather
+ # than a full replace.
+ assert "timed_refresh_immune_slices" in merged
+ assert "refresh_frequency" in merged
+
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_generate_dashboard_slug_and_css_applied(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
Review Comment:
**Suggestion:** Provide explicit parameter type annotations in this newly
added async test to meet the typing requirement for new Python code.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new async test method lacks type annotations on its parameters. That is
a direct match for the custom typing rule, so the suggestion is verified.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c69d687213814e14bb48f0ad38078fc3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c69d687213814e14bb48f0ad38078fc3&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:**
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
**Line:** 806:812
**Comment:**
*Custom Rule: Provide explicit parameter type annotations in this newly
added async test to meet the typing requirement for new Python code.
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%2F40399&comment_hash=c25fe15be11acb7372837dbb6afa95863850d37378510b34bd5fd91c72362c13&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=c25fe15be11acb7372837dbb6afa95863850d37378510b34bd5fd91c72362c13&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1112,9 +1316,20 @@ def _sanitize_dashboard_info_for_llm_context(
return DashboardInfo.model_validate(payload)
-def dashboard_serializer(dashboard: "Dashboard") -> DashboardInfo:
- from superset.mcp_service.utils.url_utils import get_superset_base_url
+def _safe_user_label(value: Any) -> str | None:
+ """Coerce a `*_by_name` model attribute to a display string or None.
+
+ The Dashboard model exposes ``created_by_name`` / ``changed_by_name``
+ as plain strings, but some serializer call sites pass through
+ objects (User instances, Mocks in tests) — defensive coercion keeps
+ the response a valid string and avoids leaking ``repr(user)``.
+ """
+ if isinstance(value, str) and value:
+ return value
+ return None
+
+def dashboard_serializer(dashboard: "Dashboard") -> DashboardInfo:
Review Comment:
**Suggestion:** Add an inline docstring immediately under
`dashboard_serializer` to document its purpose and expected behavior, since
newly introduced functions should be documented. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python function in the final file state, and it does
not include a docstring immediately below the definition. The custom rule
requires newly added functions and classes to be documented inline, so the
suggestion identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c1453b9256d24aa0b348e1b5daf0623d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c1453b9256d24aa0b348e1b5daf0623d&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/dashboard/schemas.py
**Line:** 1332:1332
**Comment:**
*Custom Rule: Add an inline docstring immediately under
`dashboard_serializer` to document its purpose and expected behavior, since
newly introduced functions should be documented.
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%2F40399&comment_hash=26d60e545a2130ec0055481fe1e9f3f74c95536e1d14152191e5d8277fe6bb0c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=26d60e545a2130ec0055481fe1e9f3f74c95536e1d14152191e5d8277fe6bb0c&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -575,6 +686,160 @@ async def
test_generate_dashboard_empty_string_title_preserved(
created = mock_dashboard_cls.return_value
assert created.dashboard_title == ""
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_generate_dashboard_position_json_override(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
+ """An explicit ``position_json`` replaces the auto-generated layout
+ in full — the tool serializes the caller's dict verbatim into the
+ dashboard's ``position_json`` column rather than calling the
+ layout-builder helper."""
+ from superset.utils import json
+
+ charts = [_mock_chart(id=1, slice_name="Sales")]
+ mock_dashboard = _mock_dashboard(id=70, title="Custom Layout
Dashboard")
+ _setup_generate_dashboard_mocks(
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ charts,
+ mock_dashboard,
+ )
+
+ custom_layout = {
+ "ROOT_ID": {"type": "ROOT", "children": ["GRID_ID"]},
+ "GRID_ID": {"type": "GRID", "children": ["ROW-custom"]},
+ "ROW-custom": {
+ "type": "ROW",
+ "children": ["CHART-1"],
+ "meta": {"background": "BACKGROUND_TRANSPARENT"},
+ },
+ "CHART-1": {
+ "type": "CHART",
+ "children": [],
+ "meta": {"chartId": 1, "width": 12, "height": 100},
+ },
+ }
+ request = {
+ "chart_ids": [1],
+ "dashboard_title": "Custom Layout Dashboard",
+ "position_json": custom_layout,
+ }
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("generate_dashboard", {"request":
request})
+
+ assert result.structured_content["error"] is None
+ created = mock_dashboard_cls.return_value
+ # position_json on the model is a JSON string; round-trip it
+ # and verify the caller's layout — not the auto-generated 2-col
+ # grid — was written.
+ stored = json.loads(created.position_json)
+ assert stored == custom_layout
+ # The auto-generated layout's HEADER/ROW ids wouldn't match
+ # `ROW-custom`; this sanity-check guards against regressions
+ # where the override silently merges with the default.
+ assert "ROW-custom" in stored
+
+ @patch("superset.models.dashboard.Dashboard")
+ @patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+ @patch("superset.db.session")
+ @pytest.mark.asyncio
+ async def test_generate_dashboard_json_metadata_overrides_shallow_merged(
+ self,
+ mock_db_session,
+ mock_find_by_id,
+ mock_dashboard_cls,
+ mcp_server,
+ ) -> None:
Review Comment:
**Suggestion:** Add type hints for all injected fixtures and mocks in this
new test method signature to make the function fully typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another newly introduced async test method with untyped parameters.
It violates the rule that new Python code should be fully typed.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b02aa61f10c4411b88d707adacfe9ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4b02aa61f10c4411b88d707adacfe9ad&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:**
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
**Line:** 755:761
**Comment:**
*Custom Rule: Add type hints for all injected fixtures and mocks in
this new test method signature to make the function fully typed.
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%2F40399&comment_hash=8f53c733abe5ef1fe2f3737a1890a8785a5c9aff717311d220bfc1e19daa8c59&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=8f53c733abe5ef1fe2f3737a1890a8785a5c9aff717311d220bfc1e19daa8c59&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]
