chore(mcp): drop legacy /superset prefix from dashboard URLs#41758
Conversation
The historical `/superset` URL prefix was removed from every view when `Superset.route_base` was set to `""`; dashboards are now canonically served at `/dashboard/<id>/`. The MCP dashboard tools still hand-built `/superset/dashboard/<id>/` URLs, which only resolve today via the temporary legacy-prefix redirect shim (removed at EOL 5.0.0). Point them at the canonical `/dashboard/<id>/` form so the returned URLs are correct without relying on the shim, matching `add_chart_to_existing_dashboard` which already uses the new form. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code Review Agent Run #533cf6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| from superset.mcp_service.dashboard.schemas import serialize_tag_object | ||
|
|
||
| dashboard_url = f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/" | ||
| dashboard_url = f"{get_superset_base_url()}/dashboard/{dashboard.id}/" |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this newly introduced local URL variable to satisfy the type-hint requirement for relevant new variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly introduced local variable in modified Python code, and it can be explicitly annotated as a string. That matches the type-hint rule for relevant variables, so the omission is a real violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/duplicate_dashboard.py
**Line:** 119:119
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local URL variable to satisfy the type-hint requirement for relevant new variables.
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| dashboard_url = ( | ||
| f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/" | ||
| ) | ||
| dashboard_url = f"{get_superset_base_url()}/dashboard/{new_dashboard.id}/" |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this newly introduced local URL variable in the exception path to keep type hints consistent in modified code. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is also a newly introduced local variable in modified Python code and it can be annotated with a string type. The omission satisfies the stated type-hint rule violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/duplicate_dashboard.py
**Line:** 205:205
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local URL variable in the exception path to keep type hints consistent in 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| dashboard_url = ( | ||
| f"{get_superset_base_url()}/superset/dashboard/{request.dashboard_id}/" | ||
| ) | ||
| dashboard_url = f"{get_superset_base_url()}/dashboard/{request.dashboard_id}/" |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this newly introduced local variable to satisfy the type-hint requirement. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new local variable is a plain string assignment in modified Python code and could be annotated as dashboard_url: str = ..., so it matches the type-hint requirement for relevant variables.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/manage_native_filters.py
**Line:** 455:455
**Comment:**
*Custom Rule: Add an explicit type annotation for this newly introduced local variable to satisfy the 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| dashboard_url = ( | ||
| f"{get_superset_base_url()}/superset/dashboard/{updated_dashboard.id}/" | ||
| ) | ||
| dashboard_url = f"{get_superset_base_url()}/dashboard/{updated_dashboard.id}/" |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for the newly introduced local variable, e.g. annotate dashboard_url as a string. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new local variable dashboard_url is introduced without an explicit type annotation, and it clearly has a concrete string type. This matches the rule requiring type hints on relevant new or modified Python variables that can be annotated.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py
**Line:** 489:489
**Comment:**
*Custom Rule: Add an explicit type annotation for the newly introduced local variable, e.g. annotate `dashboard_url` as a string.
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
| # defense), matching the standard dashboard serializers. | ||
| assert content["dashboard"]["dashboard_title"] == _wrapped("Staging Copy") | ||
| assert "/superset/dashboard/2/" in content["dashboard_url"] | ||
| assert "/dashboard/2/" in content["dashboard_url"] |
There was a problem hiding this comment.
Suggestion: This assertion is too weak: "/dashboard/2/" in ... will still pass for legacy URLs like "/superset/dashboard/2/", so the test won’t catch a regression back to the deprecated prefix. Assert the full expected path (or assert that "/superset/" is absent) to enforce the canonical URL contract. [incorrect condition logic]
Severity Level: Major ⚠️
⚠️ duplicate_dashboard happy-path test may miss URL regressions.
⚠️ MCP dashboard duplication URLs could break undetected.Steps of Reproduction ✅
1. Observe in `superset/mcp_service/dashboard/tool/duplicate_dashboard.py:115-149` that
`_serialize_new_dashboard()` builds `dashboard_url =
f"{get_superset_base_url()}/dashboard/{dashboard.id}/"` and returns it in the tool
response.
2. In `tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:28-41`,
`test_duplicate_referencing_same_charts` calls the `duplicate_dashboard` MCP tool via
`Client.call_tool("duplicate_dashboard", ...)` and asserts `"/dashboard/2/" in
content["dashboard_url"]`.
3. Note that a legacy URL like `f"{get_superset_base_url()}/superset/dashboard/2/"` (for
example if someone reintroduces the old prefix in `_serialize_new_dashboard()` at line
119) still contains the substring `"/dashboard/2/"`, so this assertion would pass even
though the URL path is non-canonical.
4. Modify `_serialize_new_dashboard()` at `duplicate_dashboard.py:119` to return
`f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"`, run `pytest
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py`, and observe
`test_duplicate_referencing_same_charts` still passes, demonstrating the test cannot
detect regressions back to the deprecated `/superset/dashboard/<id>/` path.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py
**Line:** 190:190
**Comment:**
*Incorrect Condition Logic: This assertion is too weak: `"/dashboard/2/" in ...` will still pass for legacy URLs like `"/superset/dashboard/2/"`, so the test won’t catch a regression back to the deprecated prefix. Assert the full expected path (or assert that `"/superset/"` is absent) to enforce the canonical URL 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| assert content["duplicated_slices"] is True | ||
| assert content["dashboard"]["id"] == 3 | ||
| assert "/superset/dashboard/3/" in content["dashboard_url"] | ||
| assert "/dashboard/3/" in content["dashboard_url"] |
There was a problem hiding this comment.
Suggestion: This check has the same false-positive issue: a legacy "/superset/dashboard/3/" URL still contains "/dashboard/3/", so the test can pass even when behavior is wrong after the redirect shim is removed. Make the assertion strict on the full URL shape. [incorrect condition logic]
Severity Level: Major ⚠️
⚠️ duplicate_dashboard duplicate_slices test may miss URL bugs.
⚠️ Canonical dashboard URL contract weakly enforced in tests.Steps of Reproduction ✅
1. In `superset/mcp_service/dashboard/tool/duplicate_dashboard.py:115-149`, confirm
`_serialize_new_dashboard()` constructs `dashboard_url` using
`f"{get_superset_base_url()}/dashboard/{dashboard.id}/"` and returns it as part of the
`DuplicateDashboardResponse`.
2. In `tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:60-91`,
`test_duplicate_with_duplicate_slices` calls the same tool and asserts `"/dashboard/3/" in
content["dashboard_url"]` to validate the response URL.
3. Recognize that a legacy-style URL such as
`f"{get_superset_base_url()}/superset/dashboard/3/"` would still include the substring
`"/dashboard/3/"`, so the assertion at line 240 would pass even if the tool regressed to
emitting `/superset/dashboard/<id>/` instead of the canonical `/dashboard/<id>/`.
4. Change the implementation at `duplicate_dashboard.py:119` to build `dashboard_url =
f"{get_superset_base_url()}/superset/dashboard/{dashboard.id}/"`, run `pytest
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py`, and observe
`test_duplicate_with_duplicate_slices` still succeeds, showing that the current substring
assertion cannot detect a reversion to deprecated URL prefixes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py
**Line:** 240:240
**Comment:**
*Incorrect Condition Logic: This check has the same false-positive issue: a legacy `"/superset/dashboard/3/"` URL still contains `"/dashboard/3/"`, so the test can pass even when behavior is wrong after the redirect shim is removed. Make the assertion strict on the full URL shape.
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| assert content["dashboard"]["id"] == 7 | ||
| assert content["dashboard"]["dashboard_title"] == _wrapped("Copy") | ||
| assert "/superset/dashboard/7/" in content["dashboard_url"] | ||
| assert "/dashboard/7/" in content["dashboard_url"] |
There was a problem hiding this comment.
Suggestion: This assertion also only checks containment, so "/superset/dashboard/7/" would satisfy it and mask an incorrect response format. Tighten the expectation to validate canonical dashboard URLs explicitly. [incorrect condition logic]
Severity Level: Major ⚠️
⚠️ Fallback duplicate_dashboard response URL regressions may pass tests.
⚠️ MCP error-path URL correctness weakly validated in suite.Steps of Reproduction ✅
1. Inspect `_refetch_and_serialize()` in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:171-213`; in the fallback
error-handling path it sets `dashboard_url =
f"{get_superset_base_url()}/dashboard/{new_dashboard.id}/"` and returns it alongside a
minimal `DashboardInfo`.
2. In `tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py:269-303`,
`test_refetch_failure_rolls_back_and_returns_minimal_response` simulates a re-fetch
`SQLAlchemyError`, triggering this fallback path, and then asserts `"/dashboard/7/" in
content["dashboard_url"]` to validate the URL in the minimal response.
3. If `_refetch_and_serialize()` were to build a legacy URL like
`f"{get_superset_base_url()}/superset/dashboard/{new_dashboard.id}/"` (for example by
accidentally reusing the deprecated prefix at line 205), the resulting string
`".../superset/dashboard/7/"` would still contain `"/dashboard/7/"`, so the assertion at
line 452 would pass even though the tool is no longer emitting the canonical route.
4. Apply that change at `duplicate_dashboard.py:205`, re-run `pytest
tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py`, and confirm that
`test_refetch_failure_rolls_back_and_returns_minimal_response` continues to pass,
demonstrating that the test’s non-strict containment check cannot detect regressions in
the fallback URL format.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py
**Line:** 452:452
**Comment:**
*Incorrect Condition Logic: This assertion also only checks containment, so `"/superset/dashboard/7/"` would satisfy it and mask an incorrect response format. Tighten the expectation to validate canonical dashboard URLs explicitly.
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| assert content["permission_denied"] is False | ||
| assert content["dashboard_url"] is not None | ||
| assert "/superset/dashboard/1/" in content["dashboard_url"] | ||
| assert "/dashboard/1/" in content["dashboard_url"] |
There was a problem hiding this comment.
Suggestion: This URL assertion is similarly permissive and will pass for legacy prefixed paths, so it does not actually verify that the tool emits the new canonical route. Assert the exact expected URL pattern to prevent false positives. [incorrect condition logic]
Severity Level: Major ⚠️
⚠️ remove_chart_from_dashboard URL regressions may escape detection.
⚠️ MCP dashboard URL contract not strictly enforced by tests.Steps of Reproduction ✅
1. Review `remove_chart_from_dashboard()` in
`superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py:52-257`; both the
SQLAlchemyError fallback at lines 161-175 and the main success path at lines 227-257 build
`dashboard_url = f"{get_superset_base_url()}/dashboard/{updated_dashboard.id}/"` and
return it in `RemoveChartFromDashboardResponse`.
2. In
`tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py:29-56`,
`test_simple_grid_removal_prunes_empty_row` exercises the happy-path removal and asserts
`assert "/dashboard/1/" in content["dashboard_url"]` after calling the `_call_remove`
helper.
3. A legacy URL such as `f"{get_superset_base_url()}/superset/dashboard/1/"` (for example
if someone reintroduces the `/superset` prefix into `dashboard_url` construction at line
257) still contains the substring `"/dashboard/1/"`, so the assertion at line 374 would
pass even though the MCP tool is emitting non-canonical, shim-dependent routes.
4. Change `dashboard_url` builders in `remove_chart_from_dashboard.py` at lines 162, 237,
and 257 to include `/superset/dashboard/` instead of `/dashboard/`, run `pytest
tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py`, and
observe `test_simple_grid_removal_prunes_empty_row` still passing, confirming that the
substring assertion cannot detect regressions to the deprecated prefixed paths.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_remove_chart_from_dashboard.py
**Line:** 374:374
**Comment:**
*Incorrect Condition Logic: This URL assertion is similarly permissive and will pass for legacy prefixed paths, so it does not actually verify that the tool emits the new canonical route. Assert the exact expected URL pattern to prevent false positives.
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41758 +/- ##
==========================================
- Coverage 64.66% 64.66% -0.01%
==========================================
Files 2685 2685
Lines 148533 148533
Branches 34268 34268
==========================================
- Hits 96054 96047 -7
- Misses 50720 50724 +4
- Partials 1759 1762 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sadpandajoe
added
the
merge-if-green
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Verified against current routing: superset/views/dashboard/views.py sets route_base = "/dashboard", and the PR-referenced legacy_prefix_redirect shim confirms /superset/dashboard/<id>/ is the temporary (EOL 5.0.0) redirect target, not the canonical path — so /dashboard/<id>/ is correct.
Completeness check: grepped all of mcp_service/ for remaining /superset/-prefixed URL construction — none found. Chart/explore/sql_lab tools already emit the unprefixed form, so this closes out the last dashboard-URL holdouts rather than leaving a partial migration.
No double-slash/base-URL-join regression — get_superset_base_url() already .rstrip("/")s before the f-string join.
Scans 1–20 run on the 4 changed .py files (pure string-literal diff, no new logic) — no hits beyond what's already flagged on this thread. Not duplicating codeant's open findings (weak in-based URL assertions at test_duplicate_dashboard.py:190/240/452 and test_remove_chart_from_dashboard.py:374; type-annotation nits on the 4 new dashboard_url locals) — those stand on their own threads.
schemas.py docstring update matches the new path. No other test in the suite asserts the old prefix for these tools.
3 participants
SUMMARY
The historical
/supersetURL prefix was removed from every view whenSuperset.route_basewas set to""— dashboards are now canonically served at/dashboard/<id>/. A temporary closed-set redirect shim (superset/middleware/legacy_prefix_redirect.py, removed at EOL 5.0.0) keeps the old/superset/dashboard/<id>/paths working for one release cycle.The MCP dashboard tools still hand-build
/superset/dashboard/<id>/URLs in their responses, so the URLs they return only resolve today by bouncing through that temporary shim. This updates them to emit the canonical/dashboard/<id>/form directly, so the returned URLs are correct on their own and keep working after the shim is retired.This matches
add_chart_to_existing_dashboard, which already returns the new/dashboard/<id>/form.Files touched (all pure string changes, no behavioral logic):
mcp_service/dashboard/tool/duplicate_dashboard.pymcp_service/dashboard/tool/manage_native_filters.pymcp_service/dashboard/tool/remove_chart_from_dashboard.pymcp_service/dashboard/schemas.py(field help text)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — response URL string change only.
TESTING INSTRUCTIONS
Existing unit tests updated to assert the
/dashboard/<id>/form:ADDITIONAL INFORMATION