codeant-ai-for-open-source[bot] commented on code in PR #40399:
URL: https://github.com/apache/superset/pull/40399#discussion_r3422470717
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,216 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestGenerateDashboardRequestLayoutTheme:
+ """generate_dashboard accepts optional position_json, theme overrides,
CSS."""
+
+ def test_layout_theme_css_fields_default_to_none(self) -> None:
Review Comment:
**Suggestion:** Add a short docstring describing the behavior this new test
validates. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The method is newly added and has no docstring. The custom rule explicitly
requires new Python functions and classes to include docstrings, so this is a
verified violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b4ae7f94e7e849f9abbffaef7b0a5c89&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=b4ae7f94e7e849f9abbffaef7b0a5c89&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/test_dashboard_schemas.py
**Line:** 635:635
**Comment:**
*Custom Rule: Add a short docstring describing the behavior this new
test validates.
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=3adb429644dc3f8163b0c6d7490cd27197843f8f79d935a139af661101c16cce&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=3adb429644dc3f8163b0c6d7490cd27197843f8f79d935a139af661101c16cce&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -537,8 +539,8 @@ def test_builder_none_values(self):
assert "empty" in result["empty_field"]
assert "empty" in result["also_empty"]
- @patch("superset.mcp_service.utils.url_utils.get_superset_base_url")
- def test_omitted_fields_in_serialized_dashboard(self, mock_base_url):
+ @patch("superset.mcp_service.dashboard.schemas.get_superset_base_url")
+ def test_omitted_fields_in_serialized_dashboard(self, mock_base_url) ->
None:
Review Comment:
**Suggestion:** Add an explicit type annotation for `mock_base_url` (for
example `MagicMock`) so the new test method has fully typed parameters.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added test method, and its patched parameter `mock_base_url`
is not type-annotated. The custom rule requires new or modified Python
functions/methods to be fully typed, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3dad1b28143c46fca62a1b8789396b32&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=3dad1b28143c46fca62a1b8789396b32&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/test_dashboard_schemas.py
**Line:** 542:543
**Comment:**
*Custom Rule: Add an explicit type annotation for `mock_base_url` (for
example `MagicMock`) so the new test method has fully typed parameters.
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=5670bc9f15ccbb018e825aa03bb71cc9dffbfbc815ea21bbcdd0b438b4ec240b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=5670bc9f15ccbb018e825aa03bb71cc9dffbfbc815ea21bbcdd0b438b4ec240b&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,216 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestGenerateDashboardRequestLayoutTheme:
+ """generate_dashboard accepts optional position_json, theme overrides,
CSS."""
+
+ def test_layout_theme_css_fields_default_to_none(self) -> None:
+ req = GenerateDashboardRequest(chart_ids=[1])
+ assert req.position_json is None
+ assert req.json_metadata_overrides is None
+ assert req.css is None
+ assert req.slug is None
+
+ def test_position_json_accepted(self) -> None:
+ position = {
+ "ROOT_ID": {"children": ["GRID_ID"], "type": "ROOT"},
+ "GRID_ID": {"children": ["ROW-1"], "type": "GRID"},
+ }
+ req = GenerateDashboardRequest(chart_ids=[1], position_json=position)
+ assert req.position_json == position
+
+ def test_json_metadata_overrides_accepted(self) -> None:
+ overrides = {
+ "label_colors": {"Electronics": "#4C78A8"},
+ "cross_filters_enabled": False,
+ }
+ req = GenerateDashboardRequest(
+ chart_ids=[1], json_metadata_overrides=overrides
+ )
+ assert req.json_metadata_overrides == overrides
+
+ def test_css_accepted(self) -> None:
+ req = GenerateDashboardRequest(
+ chart_ids=[1], css=".header-controls{display:none}"
+ )
+ assert req.css == ".header-controls{display:none}"
+
+ def test_slug_accepted(self) -> None:
+ req = GenerateDashboardRequest(chart_ids=[1], slug="my-dashboard")
+ assert req.slug == "my-dashboard"
+
+ def test_css_max_length_enforced(self) -> None:
+ with pytest.raises(ValidationError, match="at most 50000"):
+ GenerateDashboardRequest(chart_ids=[1], css="x" * 50001)
+
+ def test_title_alias_accepted(self) -> None:
+ """``title`` is one of the AliasChoices for ``dashboard_title``
+ — JSON callers using either name resolve to the same field."""
+ req = GenerateDashboardRequest(chart_ids=[1], title="Q4 Review")
+ assert req.dashboard_title == "Q4 Review"
+
+ def test_name_alias_accepted(self) -> None:
+ """``name`` is the third AliasChoice for ``dashboard_title``
+ and must resolve identically to ``title`` and ``dashboard_title``."""
+ req = GenerateDashboardRequest(chart_ids=[1], name="Q4 Review")
+ assert req.dashboard_title == "Q4 Review"
+
+ def test_published_defaults_to_false(self) -> None:
+ """``published`` defaults to False — newly generated dashboards
+ are drafts by default to avoid accidentally publishing partial
+ work-in-progress to all users."""
+ req = GenerateDashboardRequest(chart_ids=[1])
+ assert req.published is False
+
+ def test_published_true_accepted(self) -> None:
+ """An explicit ``published=True`` is preserved."""
+ req = GenerateDashboardRequest(chart_ids=[1], published=True)
+ assert req.published is True
+
+
+class TestUpdateDashboardRequest:
+ """Schema validation for update_dashboard's request."""
+
+ def test_identifier_required(self) -> None:
Review Comment:
**Suggestion:** Add a concise docstring for this newly added test so all new
functions are documented inline. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new test method lacks a docstring. Since the rule requires newly added
functions and classes to be documented inline, the violation is real.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=839b93f49491460b91dec530b93b0b3c&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=839b93f49491460b91dec530b93b0b3c&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/test_dashboard_schemas.py
**Line:** 702:702
**Comment:**
*Custom Rule: Add a concise docstring for this newly added test so all
new functions are documented inline.
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=92834660c8f366b554119baf710c3eb6600c081c7be62938bfebb4b27cf5133f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=92834660c8f366b554119baf710c3eb6600c081c7be62938bfebb4b27cf5133f&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -625,3 +627,216 @@ def
test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
assert len(req.sanitization_warnings) == 1
assert "dashboard_title" in req.sanitization_warnings[0]
assert "injected" not in req.sanitization_warnings[0]
+
+
+class TestGenerateDashboardRequestLayoutTheme:
+ """generate_dashboard accepts optional position_json, theme overrides,
CSS."""
+
+ def test_layout_theme_css_fields_default_to_none(self) -> None:
+ req = GenerateDashboardRequest(chart_ids=[1])
+ assert req.position_json is None
+ assert req.json_metadata_overrides is None
+ assert req.css is None
+ assert req.slug is None
+
+ def test_position_json_accepted(self) -> None:
Review Comment:
**Suggestion:** Add an inline docstring to this new test function to comply
with the documentation rule for added functions. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new test function without a docstring. That matches the docstring
requirement for newly added Python functions, so the suggestion is verified.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7962f66f4e034700b14ecfaf12f10278&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=7962f66f4e034700b14ecfaf12f10278&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/test_dashboard_schemas.py
**Line:** 642:642
**Comment:**
*Custom Rule: Add an inline docstring to this new test function to
comply with the documentation rule for added functions.
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=30c117d348b18dab36836fc681e15e5ea178ef75d9581c1140317bcbc7655ff7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=30c117d348b18dab36836fc681e15e5ea178ef75d9581c1140317bcbc7655ff7&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -575,6 +686,166 @@ 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 missing parameter type annotations in this newly added
method to comply with the typed-function requirement for modified code.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new async test method and its parameters are not annotated.
That directly violates the fully-typed new Python code requirement.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90c71191eb9948b3b643acf49d5dad25&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=90c71191eb9948b3b643acf49d5dad25&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:** 757:763
**Comment:**
*Custom Rule: Add missing parameter type annotations in this newly
added method to comply with the typed-function requirement for modified 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=4f23bf71549441a1e9023506aa221cca913a11159bd50e7f42c8109d2c84b5a8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=4f23bf71549441a1e9023506aa221cca913a11159bd50e7f42c8109d2c84b5a8&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py:
##########
@@ -575,6 +686,166 @@ 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:
Review Comment:
**Suggestion:** Annotate each method parameter with an explicit type (for
fixtures and mocks) to keep this newly introduced function fully typed.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly introduced test method has untyped fixture/mock parameters.
The rule explicitly requires new Python functions to be fully typed, so
the suggestion matches a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c55e436d327b4b3aa21777561259024c&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=c55e436d327b4b3aa21777561259024c&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:** 693:699
**Comment:**
*Custom Rule: Annotate each method parameter with an explicit type (for
fixtures and mocks) to keep this newly introduced 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=97b39fdd6d0788ad6b6f8b59b10be14d0e9811407159d6a09a0487c661dceb4d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=97b39fdd6d0788ad6b6f8b59b10be14d0e9811407159d6a09a0487c661dceb4d&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:
##########
@@ -555,8 +557,8 @@ def test_omitted_fields_in_serialized_dashboard(self,
mock_base_url):
assert "extracted" in result.omitted_fields["json_metadata"]
assert "layout tree" in result.omitted_fields["position_json"].lower()
- @patch("superset.mcp_service.utils.url_utils.get_superset_base_url")
- def test_omitted_fields_with_none_values(self, mock_base_url):
+ @patch("superset.mcp_service.dashboard.schemas.get_superset_base_url")
+ def test_omitted_fields_with_none_values(self, mock_base_url) -> None:
Review Comment:
**Suggestion:** Add a type annotation for `mock_base_url` in this new
patched test method to satisfy the full type-hint requirement. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This new patched test method also leaves `mock_base_url` untyped. Since the
rule says newly added Python code should be fully typed, the suggestion
correctly identifies an actual violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4cf68d7caf334499b1efc67120d3f2cb&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=4cf68d7caf334499b1efc67120d3f2cb&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/test_dashboard_schemas.py
**Line:** 560:561
**Comment:**
*Custom Rule: Add a type annotation for `mock_base_url` in this new
patched test method to satisfy the full type-hint requirement.
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=e5b4815c413ab67cbf16eef77b7397c43539873ea8960d78df53c58a3649c755&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=e5b4815c413ab67cbf16eef77b7397c43539873ea8960d78df53c58a3649c755&reaction=dislike'>👎</a>
##########
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:
Review Comment:
**Suggestion:** Add explicit type annotations for all parameters in this new
test method (including fixture and mock parameters) to satisfy the fully-typed
new code requirement. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly added Python test method, and its fixture/mock parameters
(`mock_db_session`, `mock_dashboard_cls`, `mcp_server`) are untyped.
The custom rule requires new Python code to be fully typed, so the
suggestion correctly identifies a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=80664881fa47448a8ce05e264fca2ab4&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=80664881fa47448a8ce05e264fca2ab4&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:** 467:472
**Comment:**
*Custom Rule: Add explicit type annotations for all parameters in this
new test method (including fixture and mock parameters) to satisfy the
fully-typed new code requirement.
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=24aa8b8822589adb56f9d885382d348f58a88d4e5dbc7209997ba722bef5167c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=24aa8b8822589adb56f9d885382d348f58a88d4e5dbc7209997ba722bef5167c&reaction=dislike'>👎</a>
##########
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 return type are fully typed per the rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added async test method omits type hints for its parameters.
Since the surrounding code is being changed and new Python code should
be fully typed, this is a genuine violation of the typing rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=68da1d1124254d4f9dded62610929452&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=68da1d1124254d4f9dded62610929452&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:** 530:535
**Comment:**
*Custom Rule: Add parameter type hints to this new async test method so
both inputs and return type are fully typed per the rule.
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=163430cc1528ac1d6710d453231a5919921c77638bfbc8bd1220bdb0e047d56d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=163430cc1528ac1d6710d453231a5919921c77638bfbc8bd1220bdb0e047d56d&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]
