Repository navigation
Conversation
Code Review Agent Run #ac7f0fActionable Suggestions - 0Additional Suggestions - 1
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 AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44188 +/- ##
==========================================
+ Coverage 80.25% 80.27% +0.01%
==========================================
Files 2927 2927
Lines 173273 173276 +3
Branches 40173 40175 +2
==========================================
+ Hits 139060 139094 +34
+ Misses 31618 31578 -40
- Partials 2595 2604 +9
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:
|
|
Clarified the limited scope in e494e56. All 170 chart utility tests and pre-commit checks pass. The cancelled CI job had passed its tests; the new commit triggers a fresh run. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #96d568Actionable 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks for this fix @dennisimoo, and welcome! The direction is exactly right and the tests are well constructed, but I verified on this branch that the preservation never triggers on the real update_chart / update_chart_preview paths because column normalization round-trips the config and marks every field as set before the merge runs. Details and two suggested fixes in the inline comment.
Separately, the Python CI jobs on this run look unrelated to your change (none of the failures touch mcp_service, and the alembic multiple-heads error matches a master-side CI issue that was fixed later the same day), so a rebase should clear them. We will need them green before merging either way.
e494e56 to
05c6a59
Compare
Code Review Agent Run #5f5484Actionable 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 |
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Thanks @dennisimoo, this addresses my change request. Verified on 05c6a59:
- All 13 registered plugins now dump with
model_dump(exclude_unset=True), somodel_fields_setsurvivesDatasetValidator.normalize_column_names. I swept every registered chart type on this head: minimal config, throughnormalize_column_nameswith a stubbedDatasetContext, mapped, then merged against a savedcolor_scheme="lyftColors"/row_limit=42. At my previous review SHAe494e56, 11 of the 13 lost at least one of the two saved values; on this head all 13 keep them. test_merge_chart_preserves_omitted_defaultsis the normalized path test I asked for, and it does fail ate494e56(supersetColors/100instead oflyftColors/42), so it pins the fix and not just the invariant.tests/unit_tests/mcp_service/chart/: 1638 passed, 1 skipped locally, matching your numbers. The revived"filters" not in fields_setgate is covered by the omitted/empty/null parametrization across all 13 plugins.- CI is green on this head.
Leaving the remaining presentation defaults to #44176 is the right call for this PR.
LGTM.
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
Heya @dennisimoo, the exclude_unset=True fix is right, checked it against every plugin file this touches, the direct config_dict[key] indexing later in each one is always gated behind a .get(key) truthy check first, so dropping unset defaults from the dump can't introduce a KeyError. Good root cause too, gabotorresruiz's read on update_chart re-materializing all fields through the old model_dump() was exactly it.
Approving.
(cherry picked from commit cf10e89)
3 participants
SUMMARY
Preserve saved
color_schemeandrow_limitwhen a same-visualization, same-dataset MCP chart update omits them. Explicit values, including defaults andcolor_scheme=None, still override saved values.Non-Gauge column normalizers use
model_dump(exclude_unset=True), matching Gauge, so normalization preserves omission information before both update tools merge form data. This also restores the existing omitted-filter check. Chart-specific presentation defaults remain outside this bounded fix for #44176.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Backend-only. After normalization, a Pie metric update previously reset saved
lyftColors / 42tosupersetColors / 100; it retains the saved values with this fix.TESTING INSTRUCTIONS
pytest -q tests/unit_tests/mcp_service/chart/: 1,638 passed, 1 skipped.ADDITIONAL INFORMATION
AI-assisted with Codex; reproduced and tested locally. No independent human review is claimed.