fix(chart-customizations): support migration of dynamic group by#37176
Conversation
|
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 · |
Code Review Agent Run #e96f10Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| @@ -96,8 +100,16 @@ const selectChartCustomizationConfiguration = createSelector( | |||
| state.dashboardInfo.metadata?.chart_customization_config || EMPTY_ARRAY, | |||
| selectDashboardChartIds, | |||
There was a problem hiding this comment.
Suggestion: The selector accesses state.dashboardInfo.metadata without checking whether state.dashboardInfo exists; if dashboardInfo is undefined this will throw at runtime. Add optional chaining on dashboardInfo so the selector safely falls back to EMPTY_ARRAY when dashboardInfo is not present. [null pointer]
Severity Level: Critical 🚨
- ❌ Dashboard render crashes on missing dashboardInfo.
- ❌ Chart customizations sidebar fails to mount.
- ⚠️ Unit tests mocking partial store may fail.| selectDashboardChartIds, | |
| state.dashboardInfo?.metadata?.chart_customization_config || EMPTY_ARRAY, |
Steps of Reproduction ✅
1. Open the dashboard UI component that uses chart customizations which calls
useChartCustomizationConfiguration (see useChartCustomizationConfiguration at
superset-frontend/src/dashboard/components/nativeFilters/state.ts:127).
2. During initial render the selector selectChartCustomizationConfiguration is executed
(defined at superset-frontend/src/dashboard/components/nativeFilters/state.ts:100-101).
3. If Redux state has no dashboardInfo key (state.dashboardInfo === undefined) the
expression at superset-frontend/src/dashboard/components/nativeFilters/state.ts:101
attempts to read `.metadata` on undefined and throws a TypeError.
4. Reproduce in tests by mocking react-redux useSelector to return a store object without
dashboardInfo and rendering any component that calls useChartCustomizationConfiguration;
the component render will throw at the selector line shown above.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/state.ts
**Line:** 101:101
**Comment:**
*Null Pointer: The selector accesses `state.dashboardInfo.metadata` without checking whether `state.dashboardInfo` exists; if `dashboardInfo` is undefined this will throw at runtime. Add optional chaining on `dashboardInfo` so the selector safely falls back to `EMPTY_ARRAY` when `dashboardInfo` is not present.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| const hasLegacyFormat = rawChartCustomizationConfig.some(item => | ||
| isLegacyChartCustomizationFormat(item), | ||
| ); | ||
|
|
||
| const chartCustomizationConfig = hasLegacyFormat | ||
| ? migrateChartCustomizationArray(rawChartCustomizationConfig) | ||
| : (rawChartCustomizationConfig as ChartCustomization[]); |
There was a problem hiding this comment.
Suggestion: Logic bug / unsafe migration: when any legacy item exists the code calls migrateChartCustomizationArray with the entire rawChartCustomizationConfig (including already-modern items), potentially re-migrating or corrupting items that are already in the new format; instead, migrate only legacy items and preserve existing new items (while keeping original order). [possible bug]
Severity Level: Critical 🚨
- ❌ Dashboard hydration can corrupt chart customization entries.
- ❌ Sidebar chart customization shows incorrect items.
- ⚠️ Affects dashboards with mixed-format customization arrays.
- ⚠️ Migration may alter saved dashboard configuration.| const hasLegacyFormat = rawChartCustomizationConfig.some(item => | |
| isLegacyChartCustomizationFormat(item), | |
| ); | |
| const chartCustomizationConfig = hasLegacyFormat | |
| ? migrateChartCustomizationArray(rawChartCustomizationConfig) | |
| : (rawChartCustomizationConfig as ChartCustomization[]); | |
| // Migrate only legacy items and preserve non-legacy items in original order. | |
| const legacyItems = rawChartCustomizationConfig.filter(item => | |
| isLegacyChartCustomizationFormat(item), | |
| ); | |
| const migratedLegacyItems = legacyItems.length | |
| ? migrateChartCustomizationArray(legacyItems) | |
| : []; | |
| // Reconstruct final array preserving order: replace legacy entries with migrated ones. | |
| let migratedIndex = 0; | |
| const chartCustomizationConfig = rawChartCustomizationConfig.map(item => | |
| isLegacyChartCustomizationFormat(item) | |
| ? (migratedLegacyItems[migratedIndex++] as ChartCustomization) | |
| : (item as ChartCustomization), | |
| ); |
Steps of Reproduction ✅
1. Open the dashboard page that triggers dashboard hydration. The HYDRATE_DASHBOARD action
is handled by the reducer at superset-frontend/src/dataMask/reducer.ts in the
HYDRATE_DASHBOARD case (lines ~220-320). The migration code runs at lines 223-232 in that
file.
2. Ensure the dashboard metadata returned from the backend includes
chart_customization_config with a mix of legacy-format and already-modern-format items
(the reducer reads metadata?.chart_customization_config at line 223).
3. When the HYDRATE_DASHBOARD action is dispatched, the reducer evaluates hasLegacyFormat
using isLegacyChartCustomizationFormat (imported from
src/dashboard/util/migrateChartCustomization) at lines 226-228.
4. Because at least one legacy item exists, the current code calls
migrateChartCustomizationArray with the entire rawChartCustomizationConfig array (line
231). If migrateChartCustomizationArray expects legacy-only inputs or transforms items
generically, this will re-process modern items and can change/corrupt them. Observe
unexpected/mutated chart customization entries after hydration in the dashboard UI
(sidebar customizations missing or altered), originating from reducer lines 223-232.
Note: This reproduces concretely by creating a dashboard metadata payload containing
mixed-format chart_customization_config and loading that dashboard so the
HYDRATE_DASHBOARD reducer branch executes (see reducer.ts lines 220-236).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dataMask/reducer.ts
**Line:** 226:232
**Comment:**
*Possible Bug: Logic bug / unsafe migration: when any legacy item exists the code calls `migrateChartCustomizationArray` with the entire `rawChartCustomizationConfig` (including already-modern items), potentially re-migrating or corrupting items that are already in the new format; instead, migrate only legacy items and preserve existing new items (while keeping original order).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.There was a problem hiding this comment.
migrateChartCustomizationArray checks if the item is legacy and only then migrates it
|
CodeAnt AI finished reviewing your PR. |
|
🎪 Showtime deployed environment on GHA for e75d556 • Environment: http://35.93.128.118:8080 (admin/admin) |
|
@sadpandajoe This workflow is deprecated! Please use the new Superset Showtime system instead:
Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@sadpandajoe Ephemeral environment spinning up at http://54.214.116.16:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
@DamianPendrak has this been tested yet or do you need QA validation for this PR? |
|
@rebecanogueira-preset I need QA validation for this PR |
e75d556 to
e492958
Compare
Code Review Agent Run #5239b6Actionable 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 |
…che#37176) (cherry picked from commit 1a77e17)
SUMMARY
The #36062 introduced changes to support more chart customization types. It does not handle the migration of the old dynamic group by. This is fixed in this PR
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
481bfa0ADDITIONAL INFORMATION