feat(mcp): support unsaved state in Explore and Dashboard tools #37183
Conversation
Add support for form_data_key (Explore) and permalink_key (Dashboard) parameters in MCP tools to retrieve unsaved/temporary state. When a user edits a chart in Explore but hasn't saved, the current state is stored with a form_data_key. Similarly, when users apply filters in a dashboard, the state can be persisted in a permalink. The MCP tools were previously unaware of these keys and would only retrieve the saved version. Changes: - Add form_data_key parameter to GetChartInfoRequest, GetChartDataRequest, and GetChartPreviewRequest schemas - Add permalink_key parameter to GetDashboardInfoRequest schema - Update get_chart_info to retrieve cached form_data when form_data_key is provided - Update get_chart_data to build query context from cached form_data - Update get_dashboard_info to retrieve filter state from permalink - Add is_unsaved_state/is_permalink_state flags to response schemas to indicate when data came from cache vs saved version Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
CodeAnt AI finished reviewing your PR. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #d14d53
Actionable Suggestions - 4
-
superset/mcp_service/chart/tool/get_chart_info.py - 2
- Blind exception catch too broad · Line 47-52
- Blind exception catch too broad · Line 132-138
-
superset/mcp_service/chart/tool/get_chart_data.py - 1
- Blind exception catch without specific type · Line 56-56
-
superset/mcp_service/dashboard/tool/get_dashboard_info.py - 1
- Blind exception catch too broad · Line 56-56
Review Details
-
Files reviewed - 5 · Commit Range:
ef20ba0..ef20ba0- superset/mcp_service/chart/schemas.py
- superset/mcp_service/chart/tool/get_chart_data.py
- superset/mcp_service/chart/tool/get_chart_info.py
- superset/mcp_service/dashboard/schemas.py
- superset/mcp_service/dashboard/tool/get_dashboard_info.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| try: | ||
| cmd_params = CommandParameters(key=form_data_key) | ||
| return GetFormDataCommand(cmd_params).run() | ||
| except Exception as e: | ||
| logger.warning("Failed to retrieve form_data from cache: %s", e) | ||
| return None |
There was a problem hiding this comment.
Avoid catching bare Exception at line 50. Catch specific exception types (e.g., KeyError, ValueError) to improve error handling clarity and prevent masking unexpected errors.
Code suggestion
Check the AI-generated fix before applying
| try: | |
| cmd_params = CommandParameters(key=form_data_key) | |
| return GetFormDataCommand(cmd_params).run() | |
| except Exception as e: | |
| logger.warning("Failed to retrieve form_data from cache: %s", e) | |
| return None | |
| try: | |
| cmd_params = CommandParameters(key=form_data_key) | |
| return GetFormDataCommand(cmd_params).run() | |
| except (KeyError, ValueError) as e: | |
| logger.warning("Failed to retrieve form_data from cache: %s", e) | |
| return None |
Code Review Run #d14d53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
| await ctx.info( | ||
| "Chart form_data overridden with unsaved state from cache" | ||
| ) | ||
| except Exception as e: | ||
| await ctx.warning( | ||
| "Failed to parse cached form_data: %s. " | ||
| "Using saved chart configuration." % (str(e),) |
There was a problem hiding this comment.
Avoid catching bare Exception at line 135. Catch specific exception types (e.g., json.JSONDecodeError, ValueError) to improve error handling clarity.
Code suggestion
Check the AI-generated fix before applying
| await ctx.info( | |
| "Chart form_data overridden with unsaved state from cache" | |
| ) | |
| except Exception as e: | |
| await ctx.warning( | |
| "Failed to parse cached form_data: %s. " | |
| "Using saved chart configuration." % (str(e),) | |
| await ctx.info( | |
| "Chart form_data overridden with unsaved state from cache" | |
| ) | |
| except (ValueError, TypeError) as e: | |
| await ctx.warning( | |
| "Failed to parse cached form_data: %s. " | |
| "Using saved chart configuration." % (str(e),) |
Code Review Run #d14d53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| try: | ||
| cmd_params = CommandParameters(key=form_data_key) | ||
| return GetFormDataCommand(cmd_params).run() | ||
| except Exception as e: |
There was a problem hiding this comment.
Replace bare Exception catch with specific exception types. Consider catching (KeyError, ValueError, TypeError) or other specific exceptions expected from GetFormDataCommand.run().
Code suggestion
Check the AI-generated fix before applying
| except Exception as e: | |
| except (KeyError, ValueError, TypeError) as e: |
Code Review Run #d14d53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
This is a code suggestion to improve exception handling by catching specific exceptions (KeyError, ValueError, TypeError) instead of the broad 'Exception' in the _get_cached_form_data function. It aims to be more precise about what errors to handle when retrieving cached form data.
superset/mcp_service/chart/tool/get_chart_data.py
except (KeyError, ValueError, TypeError) as e:
| result = GetDashboardPermalinkCommand(permalink_key).run() | ||
| # Convert TypedDict to regular dict for type compatibility | ||
| return dict(result) if result else None | ||
| except Exception as e: |
There was a problem hiding this comment.
Replace bare Exception catch with specific exception types (e.g., Exception from the command or ValueError). This prevents masking unexpected errors.
Code suggestion
Check the AI-generated fix before applying
| except Exception as e: | |
| except (ValueError, KeyError, Exception) as e: |
Code Review Run #d14d53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #37183 +/- ##
===========================================
+ Coverage 0 67.85% +67.85%
===========================================
Files 0 642 +642
Lines 0 48077 +48077
Branches 0 5261 +5261
===========================================
+ Hits 0 32622 +32622
- Misses 0 14168 +14168
- Partials 0 1287 +1287
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| try: | ||
| cmd_params = CommandParameters(key=form_data_key) | ||
| return GetFormDataCommand(cmd_params).run() | ||
| except Exception as e: |
| cached_form_data = _get_cached_form_data(request.form_data_key) | ||
|
|
||
| if cached_form_data: |
There was a problem hiding this comment.
| cached_form_data = _get_cached_form_data(request.form_data_key) | |
| if cached_form_data: | |
| if cached_form_data := _get_cached_form_data(request.form_data_key): |
| # If using cached form_data, we need to build query_context from it | ||
| if using_unsaved_state and cached_form_data_dict: | ||
| # Build query context from cached form_data (unsaved state) | ||
| from superset.common.query_context_factory import QueryContextFactory |
There was a problem hiding this comment.
Let's move imports to top-level, unless they're here to prevent circular imports.
| from superset.commands.explore.form_data.get import GetFormDataCommand | ||
| from superset.commands.explore.form_data.parameters import CommandParameters |
There was a problem hiding this comment.
Move to top-level if possible.
| try: | ||
| cmd_params = CommandParameters(key=form_data_key) | ||
| return GetFormDataCommand(cmd_params).run() | ||
| except Exception as e: | ||
| logger.warning("Failed to retrieve form_data from cache: %s", e) | ||
| return None |
| try: | ||
| result = GetDashboardPermalinkCommand(permalink_key).run() | ||
| # Convert TypedDict to regular dict for type compatibility | ||
| return dict(result) if result else None |
There was a problem hiding this comment.
result is already a dict, all you're doing here is losing type information.
It's better to make:
def _get_permalink_state(permalink_key: str) -> DashboardPermalinkValue | None:And fix whatever is calling _get_permalink_state().
| result = GetDashboardPermalinkCommand(permalink_key).run() | ||
| # Convert TypedDict to regular dict for type compatibility | ||
| return dict(result) if result else None | ||
| except Exception as e: |
2 participants
SUMMARY
Add support for
form_data_key(Explore) andpermalink_key(Dashboard) parameters in MCP tools to retrieve unsaved/temporary state.Problem:
When a user edits a chart in Explore (changes type, metrics, etc.) but doesn't save, the current state is stored with a
form_data_key. Similarly, when users apply filters in a dashboard, the state can be persisted in apermalink_key. The MCP tools were previously unaware of these keys and would only retrieve the saved version of charts/dashboards.This meant when users asked the chatbot to explain a chart they were editing, it would describe the saved version rather than what the user actually sees.
Solution:
form_data_keyparameter to chart tools (get_chart_info,get_chart_data,get_chart_preview) to retrieve unsaved chart configuration from cachepermalink_keyparameter toget_dashboard_infoto retrieve dashboard filter state from permalinkis_unsaved_state/is_permalink_stateflags to indicate when data came from cache vs saved versionChanges:
superset/mcp_service/chart/schemas.py: Addform_data_keyto request schemas, addis_unsaved_stateandform_data_keytoChartInfosuperset/mcp_service/chart/tool/get_chart_info.py: Retrieve cached form_data whenform_data_keyprovidedsuperset/mcp_service/chart/tool/get_chart_data.py: Build query context from cached form_datasuperset/mcp_service/dashboard/schemas.py: Addpermalink_keyto request schema, addfilter_stateandis_permalink_statetoDashboardInfosuperset/mcp_service/dashboard/tool/get_dashboard_info.py: Retrieve filter state from permalinkBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A - API changes only
TESTING INSTRUCTIONS
form_data_keyfrom the URLget_chart_infowith the chart ID and theform_data_keyis_unsaved_state: trueand theform_datareflects the unsaved changesFor dashboards:
permalink_keyfrom the URL (if using permalinks)get_dashboard_infowith the dashboard ID and thepermalink_keyis_permalink_state: trueandfilter_statewith the applied filtersADDITIONAL INFORMATION
🤖 Generated with Claude Code