fix(mcp): warn on invalid chart preview form data key

[richardfogaca]

SUMMARY

• Warn when update_chart_preview receives a previous form_data_key that cannot be loaded from cached form data.
• Keep preview generation successful for that case, preserving success: true, error: null, and the returned replacement preview form_data_key.
• Reuse the loaded previous form data for existing adhoc_filters preservation when the new chart config does not supply filters.
• Add unit coverage for both the invalid-key warning path and the valid-cache filter preservation path.

BEFORE AND AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable - backend MCP tool response change.

Before this change, a preview update with a fabricated previous form_data_key succeeded and returned a new preview key without indicating that previous cached chart state was unavailable.

After this change, the same successful response includes a top-level warnings entry explaining that the previous cached chart state could not be loaded and that the preview was generated from the supplied config only.

TESTING INSTRUCTIONS

• pytest tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py -q
• Manual MCP check: call update_chart_preview with a fabricated previous form_data_key, dataset id 3, a table config, generate_preview: true, and preview_formats: ["table"]; confirm the response keeps success: true, keeps error: null, returns a new preview form_data_key, and includes the missing previous cached state warning.

ADDITIONAL INFORMATION

• Has associated issue: Not applicable - no public Apache Superset issue
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

REVIEWER SUMMARY

update_chart_preview previously ignored a supplied form_data_key when the corresponding cached form data could not be loaded. The preview still succeeded and returned a replacement key, which made it look like the request updated an existing preview even though it was generated from the submitted config alone.

This change performs one explicit previous-form-data cache lookup when a key is provided. On cache hit, the existing adhoc_filters preservation path still uses the cached payload. On cache miss, the response stays successful but includes a top-level warnings entry explaining that previous cached chart state was unavailable.

Verification:

• Added focused unit coverage for invalid-key warnings and valid-cache filter preservation.
• Reproduced the invalid-key preview call and confirmed the response keeps success: true, keeps error: null, returns a new preview form_data_key, and includes the missing cached state warning.

Risk is low and scoped to the MCP chart preview response path. Rollback is reverting the single PR commit.

[codecov]

Codecov Report

❌ Patch coverage is 16.66667% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.87%. Comparing base (cb53745) to head (8852ff3).

Files with missing lines	Patch %	Lines
...set/mcp_service/chart/tool/update_chart_preview.py	16.66%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #39891      +/-   ##
==========================================
- Coverage   63.87%   63.87%   -0.01%     
==========================================
  Files        2582     2582              
  Lines      136413   136418       +5     
  Branches    31453    31454       +1     
==========================================
+ Hits        87136    87137       +1     
- Misses      47764    47768       +4     
  Partials     1513     1513              

Flag	Coverage Δ
hive	39.29% <16.66%> (-0.01%)	⬇️
mysql	59.00% <16.66%> (-0.01%)	⬇️
postgres	59.08% <16.66%> (-0.01%)	⬇️
presto	40.99% <16.66%> (-0.01%)	⬇️
python	60.53% <16.66%> (-0.01%)	⬇️
sqlite	58.72% <16.66%> (-0.01%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aminghadersohi]

Clean and well-scoped fix. The refactor from _get_old_adhoc_filters to _get_previous_form_data is a good improvement — single cache lookup instead of two, and the broader return type makes it reusable. Warning constant at module level is the right call. The warnings key is always present on the success path (consistently [] or populated), and absent on error returns — that is a coherent contract since callers should gate on success: true before reading it. New tests cover both the invalid-key warning path and the valid-cache filter-preservation path with correct mock ordering. LGTM.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR updates the update_chart_preview MCP tool to explicitly detect when a supplied previous form_data_key can’t be loaded from cache, returning a successful preview response while adding a top-level warning, and extends unit tests to cover both cache-miss warnings and cache-hit adhoc filter preservation.

Changes:

• Add a cache lookup for the previous form_data_key and emit a warning on cache miss while keeping the preview update successful.
• Reuse the loaded previous form data for adhoc filter preservation when the new config doesn’t provide filters.
• Add unit tests for cache-hit JSON parsing, cache-failure behavior, warning emission, and filter preservation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
superset/mcp_service/chart/tool/update_chart_preview.py	Adds warning constant, replaces old adhoc-filter-only lookup with full previous form_data lookup, and returns warnings in the success payload.
tests/unit_tests/mcp_service/chart/tool/test_update_chart_preview.py	Adds unit coverage for previous form_data lookup behavior and for warning / filter-preservation outcomes in update_chart_preview.

[aminghadersohi]

Re-reviewing after the previous approval was dismissed. Two additional commits landed since: 80487ab (new unit tests) and 8852ff3 (Copilot-suggested wording improvement). Neither changes the core logic.

Correctness — Single cache lookup instead of two, with a clean None-sentinel for a cache miss. The previous_form_data variable is correctly scoped inside the with block (Python does not create a new namespace there), so the warnings list built inside the block is safely accessible when assembling the result dict at line 98.

Warning contract — warnings is always present and always [] on the success path; absent on error paths. Callers are expected to gate on success: true before reading it, making the contract coherent. No change warranted.

Copilot inline comment #1 (AttributeError on update_chart_preview_module.CommandException) — This concern does not apply. CommandException is imported at module level on line 30 of the PR file, so update_chart_preview_module.CommandException resolves correctly in the test.

Copilot inline comment #2 (warning text ambiguity) — Addressed by commit 8852ff3: the message now explicitly says "from the previous form_data_key" in both the first and last sentence.

Tests — All four new tests are well-structured. Mock ordering (decorator stack → parameter order) is correct. Patch targets (superset.commands.explore.form_data.get.GetFormDataCommand) are at the right import site for the lazy-import pattern used in _get_previous_form_data. LGTM.

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it