fix(dashboard): restore existingGroupBy in buildExistingColumnsSet and add null guards#39416
fix(dashboard): restore existingGroupBy in buildExistingColumnsSet and add null guards#39416sadpandajoe wants to merge 4 commits intomasterfrom
Conversation
…d add null guards PR #39356 removed existing groupby columns from buildExistingColumnsSet(), causing processGroupByCustomizations() to replace charts' base groupby instead of merging. This crashed dashboards with chart customizations (dynamic groupby). Also adds null guards in isLegacyChartCustomizationFormat() and normalizeChartCustomizationsForScopeCalculation() for legacy items with customization: null. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
In the |
There was a problem hiding this comment.
Code Review Agent Run #24052
Actionable Suggestions - 1
-
superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts - 1
- Incorrect test expectation · Line 469-469
Additional Suggestions - 1
-
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx - 1
-
Misleading test name · Line 703-703The test name incorrectly describes the item as 'legacy', but according to isLegacyChartCustomizationFormat, items with customization: null are not legacy format since customization != null is required for legacy. This could mislead developers about what the test covers.
-
Review Details
-
Files reviewed - 6 · Commit Range:
5ac2904..5ac2904- superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx
- superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
- superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts
- superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts
- superset-frontend/src/dashboard/util/migrateChartCustomization.test.ts
- superset-frontend/src/dashboard/util/migrateChartCustomization.ts
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
| createChartCustomization({ id: customizationId }), | ||
| ], | ||
| }); | ||
| expectGroupBy(result, ['series_col', 'breakdown_col']); |
There was a problem hiding this comment.
The test expects groupby to be ['series_col', 'breakdown_col'], but the current implementation for 'echarts_timeseries_line' charts sets groupby to [groupByColumns[0]], which would be ['series_col']. If the intent is to preserve the multi-column base groupby on overlap, update the implementation; otherwise, correct the expected value.
Code Review Run #24052
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
…rtCustomizations Address PR feedback — use idiomatic truthy filter instead of != null. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🎪 Showtime deployed environment on GHA for 4794961 • Environment: http://54.203.117.226:8080 (admin/admin) |
|
I pushed a small follow-up for the remaining null-hardening gaps: The normal dashboard API still rejects I also added a regression test in |
Code Review Agent Run #2e340cActionable 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 |
# Conflicts: # superset-frontend/src/dashboard/actions/chartCustomizationActions.ts
|
🎪 Showtime deployed environment on GHA for 15488b5 • Environment: http://54.245.15.26:8080 (admin/admin) |
Code Review Agent Run #befea2Actionable 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 |
SUMMARY
PR #39356 (
c3a0f2749b) removed existing groupby columns frombuildExistingColumnsSet()ingetFormDataWithExtraFilters.ts. This causedprocessGroupByCustomizations()to replace charts' base groupby instead of merging with it, crashing dashboards that use chart customizations (dynamic groupby feature).This PR:
existingGroupByinbuildExistingColumnsSetso base groupby columns are treated as existing (not overridden)isLegacyChartCustomizationFormat()to reject legacy items withcustomization: nullnormalizeChartCustomizationsForScopeCalculation()to prevent null items from reachingmigrateChartCustomization()BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no UI changes; this fixes a crash in the chart render path.
TESTING INSTRUCTIONS
npx jest --testPathPattern="getFormDataWithExtraFilters|migrateChartCustomization|DashboardContainer"— 59 tests pass including 6 new regression testsADDITIONAL INFORMATION