fix(viz): follow-ups from #41714 self-review - #42530
Merged
Merged
Conversation
…acy branch Two issues found in a post-merge self-review of the deck_multi rearchitecture: - loadLayers() had no guard against a layer fetch that resolves after deck_slices (or the visibility filter) has already changed again. A stale response could still write into subSlicesLayers/layerErrors and pollute the autozoom feature accumulator for the new, already-reset generation. Fixed with a generation counter: each loadLayers call bumps it, each in-flight fetch captures the generation it started under, and its callbacks bail out if that generation is no longer current. - deck_multi's buildQuery always returns an empty queries array (it issues no query of its own -- every layer self-fetches), so `payload` is always undefined in production. The `payload?.data?.slices`/`payload?.data?.features` reads in Multi.tsx were therefore dead code, reachable only because the Jest fixtures synthesized a payload shape the real transformProps can never produce. Removed the dead branches, made `payload` optional in DeckMultiProps to match reality, and converted the fixtures in Multi.test.tsx/Multi.color.test.tsx to mock SupersetClient.get/post and drive the real v1 fetch path instead. Added two new tests: one confirming a stale, superseded layer response is ignored rather than corrupting the current render, and one confirming autozoom correctly accumulates points across layers that resolve out of order (the scenario the original v1-autozoom fix was written for, but never had a multi-layer test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dd its first test coverage The draggable percent-change baseline had zero test coverage despite six separate production fixes landing against it (crash from a non-group graphic element, hardcoded colors, NaN from category-axis coercion, a baseline that reset on every rerender, unthrottled per-pointer-move setOption calls, and a chart.getOption() undefined crash on warm navigation). That last one was patched as a symptom rather than a cause: chart.getOption() reflects the live ECharts instance's internal state, which can still be empty for a tick after mount. Root-cause fix: read series data from the echartOptions prop instead -- it's already the source of truth this effect depends on, available synchronously, and removes the mount race entirely rather than working around it. Also named the drag handle's pixel-geometry magic numbers (BASELINE_HANDLE_WIDTH etc.) and added the first test suite for the interaction itself: mount position, drag-to-rebase math, snap-to-nearest, a no-op when dragging back onto the active baseline, the empty-series edge case, ondragend redraw, and graphic cleanup on unmount. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ded a slice Two related bugs found in a post-merge self-review, both stemming from multiple MigrateViz subclasses sharing one target_viz_type (e.g. both MigrateLineChart and MigrateCompareChart migrate onto echarts_timeseries_line): - MigrateViz.downgrade()'s SQL filter (target_viz_type + a form_data_bak marker) is coarse by necessity and matches slices from ANY subclass sharing that target. Running e.g. `compare`'s downgrade would therefore also revert already-settled `line`-sourced slices back to the legacy `line` viz_type -- a plugin this same PR deletes. Each row's own form_data_bak was still correct (no data corruption), but the migration wasn't independently revertible the way its name implies. Fixed by checking form_data_bak["viz_type"] == cls.source_viz_type in downgrade_slice() before touching a row -- the backup already records which migration produced it, no new field needed. - The `superset viz-migrations downgrade --id` CLI path had the same root cause one level up: PREVIOUS_VERSION was keyed by target_viz_type, so building it from MIGRATIONS silently let the last-registered subclass for a shared target win the dict, and `migrate_by_id` would invoke the wrong class's downgrade_slice for any chart from the losing subclass. With the fix above, that would have started silently no-op'ing instead of misfiring, which just relocated the bug. Removed PREVIOUS_VERSION entirely and look the migration up by the slice's own backed-up source viz_type via MIGRATIONS (already correctly keyed 1:1 by source type). Added a regression test exercising the exact MigrateLineChart / MigrateCompareChart collision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…overlap bug Bullet's dynamic legend-row estimation (added to fix a legend-overlapping- plot bug) was local to its own transformProps.ts. TimePivot has the exact same class of bug: with period_limit unset a chart can have 50+ series (current + many priors), and its grid.top was a fixed constant regardless of legend size, so a wide legend can wrap onto extra rows with nothing reserving space for them. Extracted the estimator into utils/legendLayout.ts as estimateWrappedLegendRowCount() (also naming its previously-inline magic numbers), reused it from Bullet, and wired it into TimePivot's grid.top so extra wrapped rows get extra height instead of overlapping the plot. TimePivot's single-row case is unchanged (same fixed padding as before). Added tests for the shared helper itself, and for TimePivot: a large legend reserves more grid.top than a small one, and a hidden legend reserves none of the extra space. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…igrated buildQuery.ts partition, paired-t-test, and parallel-coordinates each independently reimplemented the same ~15 lines of legacy-parity logic: resolve a sort metric from timeseries_limit_metric, append it to the selected metrics if missing, and build the orderby tuple. The three differ only in two real, legacy-behavior-derived ways (verified against master:superset/viz.py before extracting): whether an unset sort metric falls back to the first selected metric (partition: yes; the other two: no), and whether ordering only happens when order_desc is explicitly set (paired-t-test/ parallel-coordinates: yes; partition always orders, just flips direction). Extracted `buildSortMetricOrderby()` into @superset-ui/chart-controls/utils, parameterized on those two axes so the three charts keep their real, intentional differences rather than being forced into false uniformity. horizon was left alone -- its ordering logic is simpler (orders directly by the first metric, no timeseries_limit_metric override or metrics-list injection) and isn't actually the same shape as the other three. Also fills a test gap the extraction surfaced: paired-t-test's buildQuery tests only exercised order_desc: true with a sort metric set, never the "append to metrics but don't order" case that is the entire point of its order-gating behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EchartsRose.tsx had no component-level test at all: the click-to-drill dataIndex mapping and, more importantly, the CategoricalColorNamespace priming-order dependency (the scale must be primed in seriesNames order before building the drilled pie's slices, since getScale() returns a fresh scale each time and slices are sorted by value, a different order than seriesNames) were both unverified -- easy to silently break into wrong colors with no error. Verified the color-priming test actually catches a regression by temporarily reordering the priming call to happen after the value-sort (reproducing the exact bug class described above): the test failed as expected, then passed again once reverted. Getting there required also resetting @superset-ui/core's process-wide label-color singleton between tests -- it remembers a label's color per sliceId, so without the reset whichever test ran first would permanently decide the colors for every later test in the file, making a naive version of this test tautological (always passing regardless of priming order). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
upgrade_slice assumed a stored query_context always carries a "queries" key. An atypical one (e.g. hand-edited via the API) missing it raised a bare KeyError inside the broad except, after viz_type had already been flipped -- leaving the slice half-migrated (new viz_type, but stale params/query_context in the old shape). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
transformData.ts had thorough tests, but transformProps.ts itself -- the glue that decides between the v1 flat-record path and the legacy nested-hierarchy passthrough, maps groupby columns through verboseMap, and parses partitionLimit/Threshold -- had none. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rusackas
requested review from
betodealmeida,
eschutho,
michael-s-molina,
mistercrunch and
sadpandajoe
as code owners
July 28, 2026 08:27
dosubot
Bot
added
change:frontend
Contributor
|
Bito Automatic Review Skipped - Branch Excluded |
github-actions
Bot
added
risk:db-migration
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## remove-legacy-viz-pipeline #42530 +/- ##
==============================================================
+ Coverage 65.85% 65.91% +0.06%
==============================================================
Files 2807 2808 +1
Lines 156075 156087 +12
Branches 35849 35849
==============================================================
+ Hits 102781 102883 +102
+ Misses 51382 51292 -90
Partials 1912 1912
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:
|
- Guard the deck.gl Multi metadata-fetch effect with its own generation ref so a slower, earlier fetchSubslices call can't clobber a newer generation's loadLayers state. - Fix a divide-by-zero in Bullet transformProps that produced an Infinite marker symbolOffset for grouped empty results. - Fix a stale-resolver bug in the Multi.test.tsx stale-response test that silently left the slice-1 mock promise pending. - Rename a misleadingly-titled Rose test to match its actual assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Follow-up hardening identified during a self-review of #41714 (the legacy viz pipeline removal). Targets that branch so it can land as a stack once #41714 merges. Each commit is independent and reviewable on its own:
Multi: removed a dead code path that read apayload.data.slices/.featuresshape the v1 endpoint never returns, and fixed a real race where an out-of-order/stale async layer response could clobber a newer one — added a load-generation guard and rewrote the tests around the actual v1 fetch shape instead of the dead legacy one.chart.getOption()aftersetOption(), which is fragile and untested; switched to reading theechartOptionsprop directly and added the plugin's first test coverage for the drag-to-rebase interaction.MigrateViz.downgradematched slices bytarget_viz_typealone, so when two source viz types share one target (e.g.MigrateLineChartandMigrateCompareChartboth migrate ontoecharts_timeseries_line), downgrading one could revert slices the other had upgraded. Added a precise per-row check and removed the CLI's separate, now-inconsistentPREVIOUS_VERSIONmapping in favor of deriving the source viz type from the stored backup.partition,paired_ttest, andparallel_coordinateseach reimplemented "append the sort metric if not already selected, order by it" with slightly different rules; extracted a single shared, tested helper.seriesNamesorder, not the drilled pie's value-sorted order) -- a case that's easy to accidentally break during future refactors.upgrade_sliceassumed a storedquery_contextalways has a"queries"key; an atypical one (e.g. hand-edited via the API) missing it raised an uncaughtKeyErrorinside the broadexcept, afterviz_typehad already been flipped, leaving the slice half-migrated. Now degrades gracefully.transformProps.tsitself (as opposed totransformData.ts, which was already well covered) -- the v1-flat-record vs. legacy-nested-hierarchy branch, verboseMap level mapping, andpartitionLimit/partitionThresholdparsing.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A -- no user-visible behavior changes; this is bug fixes and test hardening on top of #41714.
TESTING INSTRUCTIONS
pytest tests/unit_tests/migrations/-- all migration unit tests, including two new regression tests (downgrade scoping, malformedquery_context)npm run test -- plugins/preset-chart-deckgl/src/Multi plugins/plugin-chart-echarts/src/utils/legendLayout.test.ts plugins/plugin-chart-echarts/test/TimePivot/transformProps.test.ts plugins/plugin-chart-echarts/test/Timeseries/EchartsTimeseries.test.tsx plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx packages/superset-ui-chart-controls/test/utils/buildSortMetricOrderby.test.ts plugins/plugin-chart-paired-t-test/test/buildQuery.test.ts plugins/plugin-chart-parallel-coordinates/test plugins/plugin-chart-partition/testquery_contextKeyErrorfix) were verified by deliberately reverting the corresponding fix and confirming the new test fails, then restoring it and confirming it passes.ADDITIONAL INFORMATION
Stacked on #41714 -- targets
remove-legacy-viz-pipeline, notmaster. Should be reviewed/merged after (or alongside) that PR.