fix(explore): add matrixify_enable guard to prevent stale validators on pre-revamp charts#38765
fix(explore): add matrixify_enable guard to prevent stale validators on pre-revamp charts#38765sadpandajoe wants to merge 7 commits intomasterfrom
Conversation
Code Review Agent Run #b767feActionable 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 |
Sequence DiagramThis PR fixes legacy chart loading by normalizing stale matrixify fields before control state is built, and by gating matrixify visibility on an explicit enable flag. Together, these changes prevent hidden validators from blocking initial queries and save actions. sequenceDiagram
participant User
participant ExploreOrDashboard
participant Store
participant MatrixifyControls
participant UI
User->>ExploreOrDashboard: Open legacy chart
ExploreOrDashboard->>Store: Normalize deprecated form data
Store->>Store: If enable missing migrate old flags or disable stale modes
ExploreOrDashboard->>MatrixifyControls: Build controls from normalized form data
MatrixifyControls->>MatrixifyControls: Require matrixify enable true before mode checks
MatrixifyControls-->>UI: No hidden matrixify validators
UI-->>User: Chart loads and save update stays enabled
Generated by CodeAnt AI |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Fixes a regression where legacy (pre-matrixify-revamp) charts can end up with hidden matrixify validation errors in Explore due to stale persisted matrixify_mode_* values, and normalizes legacy matrixify-related form_data during Explore and dashboard hydration.
Changes:
- Guard
isMatrixifyVisible()behindmatrixify_enable === trueto prevent invisible validators from firing on legacy charts. - Add
handleDeprecatedControls()migration logic to normalize legacy matrixify fields (including old per-axis enable flags) and apply it consistently across Explore + dashboard hydration paths. - Add/expand unit tests covering the guard behavior, validator injection, and form_data normalization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/src/explore/store.ts | Exports and extends deprecated-control normalization to migrate/reset legacy matrixify fields; applies normalization in applyDefaultFormData. |
| superset-frontend/src/explore/store.test.tsx | Adds regression tests for legacy matrixify normalization across Explore and dashboard paths. |
| superset-frontend/src/explore/actions/hydrateExplore.ts | Runs deprecated-control normalization before storing initial form_data in Redux state. |
| superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx | Adds matrixify_enable guard in isMatrixifyVisible() and exports it for testing. |
| superset-frontend/packages/superset-ui-chart-controls/test/shared-controls/matrixifyControls.test.tsx | New tests for visibility guard and validator injection behavior. |
| // must reflect the same migration so the two stay consistent. | ||
| handleDeprecatedControls(initialFormData); | ||
|
|
||
| const initialExploreState = { | ||
| form_data: initialFormData, | ||
| slice: initialSlice, |
There was a problem hiding this comment.
hydrateExplore mutates initialFormData in place (now including handleDeprecatedControls(initialFormData)). When initialFormData is sourced from initialSlice?.form_data (from Redux state), this can mutate the sliceEntities cache/state object. Consider cloning (and possibly deep-cloning) the initial form_data before any in-place edits/migrations so Redux state isn't mutated indirectly.
There was a problem hiding this comment.
Pre-existing pattern — lines 80–103 of this same function already mutate initialFormData in place (viz_type, time_range, groupby, granularity_sqla, dashboardId). Cloning just for this call would be inconsistent; fixing the broader mutation-of-Redux-state issue is out of scope for this bug fix.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| (window as any).featureFlags = {}; |
There was a problem hiding this comment.
This test file assigns window.featureFlags at module scope but doesn't restore the prior value. To avoid cross-test leakage when Jest reuses the same environment for multiple files, consider capturing the existing window.featureFlags in beforeAll/beforeEach and restoring it in afterAll/afterEach.
There was a problem hiding this comment.
This follows the existing codebase pattern (e.g. ExploreChartHeader.test.tsx:43 does the same). Setting to an empty {} at module scope doesn't leak test-specific state between files.
| if (!('matrixify_enable' in formData)) { | ||
| const hadVerticalLayout = | ||
| formData.matrixify_enable_vertical_layout === true; | ||
| const hadHorizontalLayout = | ||
| formData.matrixify_enable_horizontal_layout === true; | ||
|
|
||
| if (hadVerticalLayout || hadHorizontalLayout) { | ||
| // Pre-revamp chart that genuinely used matrixify — migrate to new flag | ||
| formData.matrixify_enable = true; | ||
| if (!hadVerticalLayout) formData.matrixify_mode_rows = 'disabled'; | ||
| if (!hadHorizontalLayout) formData.matrixify_mode_columns = 'disabled'; | ||
| } else { |
There was a problem hiding this comment.
handleDeprecatedControls migrates legacy per-axis matrixify flags to the new matrixify_enable toggle, but it leaves the deprecated keys (matrixify_enable_vertical_layout / matrixify_enable_horizontal_layout) on formData. Because applyDefaultFormData later merges all keys from cleanedFormData, these deprecated fields can be carried forward and re-persisted. Consider deleting these deprecated properties after migration so the normalized form_data no longer contains removed controls.
There was a problem hiding this comment.
Follows the existing convention in this function — the y_axis_zero migration (line 57–60) also leaves the deprecated key on formData after migrating its value. The old per-axis keys are dead data (no code reads them after the revamp), so leaving them is harmless and consistent.
Code Review Agent Run #aea000Actionable 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 |
| // The matrixify_enable guard prevents hidden validators from firing on | ||
| // pre-revamp charts with stale matrixify_mode defaults (fix for #38519). | ||
| const isMatrixifyVisible = ( | ||
| controls: any, |
There was a problem hiding this comment.
Just a small NIT: controls is typed as any. Given that the function only accesses controls?.[key]?.value, a narrower type like Record<string, { value?: unknown } | undefined> or something like that would be sufficient.
This aligns with the project's ongoing removal of any
Not a blocker.
|
@sadpandajoe @msyavuz Is this complementary or a duplicate of #38759? |
Code Review Agent Run #a9f5e3Actionable 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 |
The previous commit replaced `controls: any` with `ControlStateMapping` in isMatrixifyVisible but left test helpers returning incompatible types, breaking the tsc step in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This should be complimentary |
Code Review Agent Run #250285Actionable 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 |
|
🎪 Showtime deployed environment on GHA for 9fcf6d8 • Environment: http://35.87.137.255:8080 (admin/admin) |
User description
SUMMARY
Some charts are failing to load in explore view as well as the Save/Update buttons are disabled and the initial query is blocked by invisible validation errors.
Root cause:
isMatrixifyVisible()evaluates mode values (matrixify_mode_rows/matrixify_mode_columns) without first checking whether matrixify is enabled (matrixify_enable). Pre-revamp charts have stale mode defaults ('dimensions'/'metrics') persisted in form_data, soisMatrixifyVisible()returnstrueand validators fire on empty matrixify controls. The errors are invisible because matrixify tab controls are excluded from the error banner.Fix (3 parts):
Runtime guard (
matrixifyControls.tsx):isMatrixifyVisible()now checksmatrixify_enable === truebefore evaluating mode values. Handlesundefined(old charts) via!== true.Form_data migration (
store.ts):handleDeprecatedControls()normalizes stale matrixify modes on legacy charts. Detects old per-axis enable flags (matrixify_enable_vertical_layout/matrixify_enable_horizontal_layout) to preserve modes for charts that genuinely used matrixify. Required because 4 downstream UI consumers (ExploreChartPanel, ChartContextMenu, DrillBySubmenu, ChartRenderer) infer "matrixify is active" from mode values alone.Dashboard + explore consistency (
hydrateExplore.ts,store.ts):handleDeprecatedControlsexported and called inhydrateExploreandapplyDefaultFormDataso both explore and dashboard paths normalize stale form_data.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — logic-only fix with no UI changes.
TESTING INSTRUCTIONS
matrixify_enablein its form_data)Matrixifyfeature flag disabled, confirm that charts no longer hit hidden validation errors (control definitions and validators are registered regardless of the feature flag)matrixify_enable: true) — verify matrixify controls still work correctlyAutomated tests (34 total):
matrixifyControls.test.tsx(15 tests): DirectisMatrixifyVisibleguard tests + validator injection via realmapStateToPropsstore.test.tsx(19 tests): Migration, dashboard hydration, old per-axis flag detection, round-trip validationADDITIONAL INFORMATION
CodeAnt-AI Description
Fix stale Matrixify settings so saved charts load and save normally
What Changed
Impact
✅ Fewer blocked chart saves✅ Fewer chart load failures for older saved charts✅ Consistent Matrixify behavior in Explore and dashboards💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.