Repository navigation
feat(mcp): apply native dashboard filters from an MCP client - #44096
aminghadersohi merged 27 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44096 +/- ##
==========================================
+ Coverage 80.39% 80.44% +0.04%
==========================================
Files 2932 2935 +3
Lines 174525 174821 +296
Branches 40548 40606 +58
==========================================
+ Hits 140317 140639 +322
+ Misses 31538 31513 -25
+ Partials 2670 2669 -1
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:
|
gabotorresruiz
left a comment
There was a problem hiding this comment.
Took a careful pass, focused on the write-path security posture since this is a mutating tool. It holds up well:
- Read gate is the correct entitlement. The tool creates a caller-scoped permalink plus a principal-scoped realtime nudge; it never mutates saved dashboard config, so
method_permission_name="read"+class_permission_name="Dashboard"is right, andreadOnlyHint=Falsecorrectly lands it in the cacheexcluded_toolslist. Per-object access is enforced viaDashboardDAO.get_by_id_or_slug. - Embedded guests are default-denied (not in
MCP_GUEST_ALLOWED_TOOLS; theauth.pyguest gate is default-deny even with RBAC off). - No injection surface. The IN-predicate column is server-derived from the dashboard's own filter config, values are parameterized scalars placed exactly as the UI does, and
time_rangegoes throughvalidate_time_range. The1 = 0clear predicate is a hardcoded constant. - Base-permalink stacking resolves under the caller's access and rejects a base key that does not belong to the target dashboard, so no cross-dashboard or cross-principal state injection; the raw base mask is merged server-side and not returned.
Three small non-blocking items:
-
NIT — the
1 = 0clear predicate is spelled out independently in three places across two languages (getSelectExtraFormDatainfilters/utils.ts, the backend_select_data_mask, andreducer.ts). If any one drifts (1=0, a different match-nothing predicate), a cleared required filter would silently restore its saved default and nothing in the current tests would catch it. A shared exported constant, or at least a comment cross-linking the three sites, would harden this coupling. -
Question (the
realtime/publish.pyhunk you flagged) — the resolved channel is memoized onapp.extensions["realtime_channel"], so a callableREALTIME_CHANNEL_PREFIXis resolved exactly once per process and every later publisher, including this MCP path, reuses that first value. Stable and correct for the default empty prefix. Is once-and-cached the intended contract for the MCP publisher too, in a deployment where the prefix is a callable that could resolve differently by context, or should its channel be resolved per-call? -
NIT (you flagged this yourself) —
action.onClickinMessageToasts/types.tsstores a function in Redux state. I checked:messageToastsis not persisted andserializableCheckis off in the default prod middleware, so the practical fallout is limited to DevTools time-travel and the opt-in dev warning. A one-line comment noting the field is deliberately non-serializable would save the next person a double-take.
Everything I could verify statically and via the extracted publisher tests looks correct, and CI is fully green. The one thing I could not exercise is the live realtime + Undo flow, which is inert without a websocket-enabled stack and is the piece you marked pending. I would love to see those screenshots before this merges; otherwise this is clean, careful work.
|
@gabotorresruiz Thanks for the careful review! Addressed the two nits and investigated the channel contract:
Screenshots remain outstanding. This environment does not have a running websocket-enabled stack (the local Superset health endpoint is unavailable), so I cannot exercise or capture the live realtime + Undo flow. The unit tests are not a substitute for that live validation, which is still needed before merge. Local validation: 43 frontend tests and 50 backend/publisher tests passed. |
|
The issue described is a potential runtime error in the dashboard hydration process where |
|
| Language | Invalidated translations |
|---|---|
zh |
7 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (zh):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
|
Follow-up covering every review item, including the three inline comments added after the earlier reply:
Local validation: 86 focused backend/publisher tests, 41 TaskManager tests, and 51 frontend tests passed. The full PR-changed-file pre-commit gate also passed. Live realtime + Undo screenshots are still outstanding: there is no running websocket-enabled stack in this environment, so neither screenshots nor live browser verification are claimed. This remains a pre-merge validation item, not something unit tests can replace. |
|
CI follow-up: both Python unit-job attempts passed all 15,139 main tests, but exhausted the 30-minute job timeout during coverage processing. The first attempt also passed all 1,717 SQL coverage tests before cancellation. In 83d7a11, increased only that job budget to 45 minutes so the full suite, coverage finalization, and scoped coverage gates can finish. No tests, coverage thresholds, or required checks were removed or bypassed. The full PR-changed-file pre-commit gate passed again. Monitoring the new-head CI run to completion. |
…p-apply-dashboard-filters
Code Review Agent Run #6a3dfcActionable 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 |
…ghadersohi/mcp-apply-dashboard-filters
…ghadersohi/mcp-apply-dashboard-filters
|
Thanks @aminghadersohi for closing the loop on the live realtime + Undo validation, the screenshots and sequence in the validation comment are exactly what I was asking for. I also re-verified the delta since Two small things before I approve:
CI wise you are already unblocked: your latest master merge picked up the alembic heads fix from #44288, and the required checks are re running now. The one shard that failed so far ( |
|
Done — all reviewer feedback has been addressed, the PR branch is updated with current Apache master, and CI is green. |
Code Review Agent Run #90b4fdActionable Suggestions - 0Additional Suggestions - 2
Review 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 |
Address review on apply_dashboard_filters: - Drop inherited data mask entries for filters the dashboard no longer defines, so a deleted or recreated filter cannot carry a stale column predicate into a live update that falls back to every chart. - Reject more than one value for a single-select filter, which the dashboard control cannot render without rewriting the selection. - Document the browser-reachable WEBSOCKET_URL prerequisite for live updates, whose loopback default silently never connects remotely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ghadersohi/mcp-apply-dashboard-filters # Conflicts: # .github/workflows/superset-python-unittest.yml
The metadata inventory test that landed on master asserts the registered tool set exactly matches its hand-maintained lists, so this branch's new tool has to be declared there: an output field for the schema assertion and MUTATING_TOOLS membership to match its readOnlyHint=False annotation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code Review Agent Run #f049a0Actionable 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 |
gabotorresruiz
left a comment
There was a problem hiding this comment.
Re-reviewed on a24ea52161. Everything I held on last time is addressed, and I checked it by execution rather than by reading the replies:
- The amended
Toastregression test really pins the behavior now. I reverted only the!toast.actionguard inToast.tsxon this branch:an actionable toast remains until it is explicitly dismissedfails, and passes again once the guard is restored. test_single_select_rejects_multiple_valuesandtest_inherited_mask_drops_filters_the_dashboard_no_longer_hasboth fail when I restoreapply_dashboard_filters.pyfromdf68ef0d85^, and pass at head, so they are real regression tests.- Full runs on this branch: 149 backend tests across
test_apply_dashboard_filters.py,test_permalink.py,test_realtime_publish.py,test_manager.pyandtest_mcp_tool_registration.py, plus 41 frontend tests acrossToast.test.tsx,useDashboardFilterSync.test.tsxandreducer.test.ts. All green, and CI is green on the head SHA. - The
TaskManagerrefactor round trips:get_realtime_channel(app).removesuffix("realtime")reproduces the old prefix, so the channel and envelope are unchanged for the existing publishers.
Two small NITs inline, neither blocking.
One ask, also not blocking: the live browser validation is at db84fe4897, which predates 10196a117d. That commit made actionable toasts persistent and made Undo restore only the entries that still match the applied snapshot, so those screenshots show the old eight second toast rather than what ships here. The unit coverage for both changes is solid, so this is only about closing the loop: if it is cheap, a quick re-run of just the Undo flow at head would do it.
LGTM otherwise. Nice, careful work.
Address review on the dashboard filter PR: - security/manager.py kept its own "1 = 0" literal as the guest-token allow-list for extras.where. It now aliases EMPTY_FILTER_SQL_EXPRESSION, the constant the frontend producer it cites already reads, so changing the predicate cannot leave the allow-list behind and start rejecting embedded charts whose required Select filter has no value selected. - Add guest-token coverage for that coupling, written against the constant rather than its literal value. - Drop the inert duration override on the Undo toast and correct the docs: an actionable toast has no auto-dismiss timer and stays up until it is acted on or closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ghadersohi/mcp-apply-dashboard-filters
|
On the non-blocking ask about the screenshots: you're right that I could not re-run the live flow at head — this worktree has no running Superset or The behavior changes themselves are covered by the unit tests you already exercised — |
Code Review Agent Run #57673dActionable 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 |
SUMMARY
Why
An MCP client needs to apply a viewer's filter selections without redefining native filters or changing the saved dashboard for everyone. A permalink stores shareable view state using the existing authorized load path; a best-effort realtime notification can apply that state to an open dashboard without turning a transient selection into shared configuration.
What
Adds
apply_dashboard_filtersfor existing exact-match select filters without inverse selection, and time filters, with name/ID targeting and an optional base permalink for stacking selections across requests. It returns a permalink and attempts a principal-scoped notification; an open dashboard resolves that permalink, applies its selections, and offers an Undo toast that stays up until it is acted on or dismissed. Saved dashboard configuration is unchanged.Deployment requirements
The realtime half is inert unless
WEBSOCKET_ENABLEis on with a coordination backend (DISTRIBUTED_COORDINATION_CONFIG), a runningsuperset-websocket, and a strong, non-defaultWEBSOCKET_JWT_SECRETof at least 32 bytes. The MCP process and websocket transport must shareREALTIME_CHANNEL_PREFIX, and the browser user needscan_readonRealtime. With the default configuration the tool still works and returns a permalink URL.live_update_pushedmeans publication succeeded, not that a browser received or applied the update. No Global Task Framework feature flag is required.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Live realtime + Undo browser validation completed at
db84fe4897. The authenticated browser received the principal-scoped event, applied Country = France without a reload, refreshed the charts, displayed the Undo toast, and restored the prior unset filter state and chart totals when Undo was clicked. The full evidence and uncropped screenshots are in the validation comment.Note on the screenshots: they predate
10196a117d, so they show the toast as it behaved then, on an eight-second auto-dismiss timer. Since that commit an actionable toast has no timer and stays up until Undo or its close button is used, and Undo restores only the entries whose current state still matches what was applied. Both behaviors are covered by unit tests (an actionable toast remains until it is explicitly dismissed, and the Undo snapshot test inuseDashboardFilterSync.test.tsx); what the screenshots show of the applied and restored dashboard state is otherwise unchanged.Applied live — Country = France and Undo toast visible
After Undo — prior unset filter state and totals restored
TESTING INSTRUCTIONS
How to test
The core realtime + Undo path has been exercised in a browser as documented above. The broader edge-case steps below remain the manual validation runbook.
{"request":{"dashboard_id":42,"filters":[{"filter_name_or_id":"Region","values":["EMEA"]}]}}dashboard_url; check the selection and chart queries. Verify the saved native-filter configuration and another user's view remain unchanged.permalink_keyasbase_permalink_keyin a second call applying Time Range =2024-01-01 : 2025-01-01. Check that Region persists. Replace Region, clear it withvalues: [], and omit the base key to start from dashboard defaults. Also validate required-filter behavior: clearing a required select must match no rows both live and after opening its permalink, even when a saved default exists. Non-exact matching modes and inverse-selection configurations must return an explicit unsupported-mode error.Automated checks:
Validation: 86 focused backend/publisher tests, 41 existing TaskManager tests, and 51 frontend tests passed. At
db84fe4897, the live websocket + browser flow also passed: MCP publication, no-reload filter/chart update, toast display, and Undo restoration. Base/PR translation extraction and comparison passed with no translation regressions. The fulluvx pre-commit run --files <changed files>gate passed without bypassing any hooks, after formatting fixes and rebuilding stale local package declarations. No evaluation or performance run has happened; those results are pending.ADDITIONAL INFORMATION
Risk & rollback
The shared realtime publisher can affect existing task notifications, while live updates and Undo can overwrite affected filters' in-memory selections. The toast action stores a callback in Redux state, so consumers expecting serializable toast metadata deserve attention. Permalink resolution remains behind the existing dashboard access checks. Delivery is best effort, with no reconnect replay. Roll back by reverting this PR; no database migration or saved filter-configuration changes need reversal. Disabling websocket delivery stops live application but also affects other realtime consumers; permalink creation remains available.
Follow-up validation
The pre-existing failure in
tests/unit_tests/mcp_service/test_mcp_e2e_smoke.py::test_tools_call_health_check_over_real_asgi_transportis excluded from this change and needs separate investigation. Evaluation/performance measurements and the broader manual edge-case runbook remain pending; the core live browser flow and screenshots are complete as noted above.The pre-existing required-filter hydration logic treats a complete NULL-only select value (
[null]) as empty and can restore a saved default. Correcting that shared hydration behavior is deferred to a separate change.Review follow-up
Review guidance
The riskiest hunk is the extraction of
TaskManager._publish_realtimeintosuperset/realtime/publish.py: it is shared surface and deserves the most scrutiny, especially callable channel-prefix resolution, envelope compatibility, routing, and no-backend behavior. Also review the optionalToastMeta.actionfield as an additive change to a widely imported primitive. Check raw base-permalink merging and access enforcement, plus the frontend's in-flight request ordering, unmount cleanup, and Undo snapshot handling.