fix(dashboard): apply dynamic groupby display controls to scoped charts#39356
Conversation
Code Review Agent Run #3efc58Actionable 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 |
dosubot
bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes dashboard Dynamic Group By display controls so scoped charts receive correct groupby updates (and no longer get incorrect filter injections), including improved handling of legacy chart customization metadata.
Changes:
- Remove the spurious “selected column as filter value” injection path and ensure dynamic group-by selections are applied as chart dimensions.
- Normalize single-select dynamic group-by values (string → one-item array) and deduplicate chord
groupby. - Normalize/migrate legacy chart customization config before scope calculation, and add regression tests for
getFormDataWithExtraFiltersandDashboardContainer.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
superset-frontend/src/dashboard/util/getFormDataWithExtraFilters.test.ts |
Adds focused regression coverage for dynamic group-by behavior and scoping edge cases. |
superset-frontend/src/dashboard/util/charts/getFormDataWithExtraFilters.ts |
Fixes dynamic group-by form-data generation (remove filter injection, normalize single-select, chord dedupe). |
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx |
Migrates legacy chart customization items before scope calculation. |
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsx |
Adds tests validating charts-in-scope calculation for chart customizations, including legacy format migration. |
Comments suppressed due to low confidence (1)
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx:212
- The legacy normalization + calculateScopes path can incorrectly expand a legacy chart customization that was previously scoped via
chartId.migrateChartCustomization()setschartsInScope: [legacy.chartId], butcalculateScopes()ignoreschartsInScopeand recomputes it solely fromitem.scope; the migrated legacy scope defaults to{ rootPath: [DASHBOARD_ROOT_ID], excluded: [] }, which will put all charts in scope and thensetInScopeStatusOfCustomizationspersists that broadened scope back into metadata.
To preserve legacy per-chart scoping, consider special-casing legacy items that include chartId when building normalizedCustomizations (e.g., after migration, set scope.excluded to chartIds.filter(id => id !== legacy.chartId) or otherwise derive a scope that yields only the intended chart(s) in calculateScopes).
// Normalize legacy chart customizations before scope calculation.
const hasLegacy = chartCustomizations.some(
isLegacyChartCustomizationFormat,
);
const normalizedCustomizations = hasLegacy
? migrateChartCustomizationArray(chartCustomizations)
: chartCustomizations;
const scopes = calculateScopes(
normalizedCustomizations,
chartIds,
chartLayoutItems,
item => item.type === ChartCustomizationType.Divider,
).map(scope => ({
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39356 +/- ##
==========================================
+ Coverage 64.45% 64.54% +0.09%
==========================================
Files 2557 2557
Lines 132976 132985 +9
Branches 30885 30891 +6
==========================================
+ Hits 85713 85841 +128
+ Misses 45772 45653 -119
Partials 1491 1491
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run #4360c0Actionable 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 |
Code Review Agent Run #940d46Actionable 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 |
richardfogaca
requested review from
DamianPendrak,
aminghadersohi,
kgabryje and
msyavuz
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Review feedback
Nice fix — the core issue (groupby values being injected as row filters) is clearly identified and the removal of the allFilters logic is the right call. A few things I'd like to understand or see addressed:
Medium: Chord chart — contradictory code paths
processGroupByCustomizations now returns {} early for chord charts (line 331), which means the updated chord branch in applyChartSpecificGroupBy (lines 413-414) is unreachable through the dynamic groupby flow. Could you either:
- Remove the early return and rely on the
applyChartSpecificGroupBychange, or - Revert the
applyChartSpecificGroupBychord change since it's dead code?
Having both is confusing to future readers.
Low: Removing existingGroupBy from buildExistingColumnsSet
Previously, columns already in the chart's base groupby were treated as conflicts and filtered out by nonConflictingColumns. Now they pass through. The "still applies when the selected column is already in the base groupby" test covers the happy path, but could this cause duplicate columns for chart types that merge (append) rather than replace groupby? Worth a quick check.
Nit: Test cleanup pattern
The try/finally spy restore in DashboardContainer.test.tsx is thorough but verbose — an afterEach(() => jest.restoreAllMocks()) at the top of the file would simplify all five test cases.
Otherwise the test coverage is solid and the legacy normalization in normalizeChartCustomizationsForScopeCalculation looks clean.
|
hey @aminghadersohi , thanks for reviewing this PR :) The chord-specific branch is leftover from an earlier attempt to support chord in the generic dynamic groupby flow, but chord was later excluded after local testing exposed regressions. So in the current PR state that branch is dead code. I don’t think we should re-enable chord in this PR just to make that path reachable. Chord is a legacy special case: it still models source/target through groupby + columns, and the backend builds its query from those fields directly, so it’s riskier than the other charts covered here. The safer approach is to keep chord excluded in this PR and remove the unreachable branch so the intent is explicit. I also checked the existingGroupBy concern. For the chart types still participating in this flow, I don’t see an obvious duplication issue from that change, but I agree it was worth validating. The jest.restoreAllMocks() cleanup suggestion is reasonable too, and I can fold that in. What do you think? |
|
Bito Automatic Review Skipped – PR Already Merged |
…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>
SUMMARY
This PR fixes Dynamic Group By display controls not updating scoped dashboard charts correctly.
The bug had two main causes:
The selected display-control value was being treated as a row-filter value during query generation, instead of being applied as a chart dimension.
Example: selecting
statuscould produce a bogusstatus IN ("status")filter instead of updating the chartgroupby.Legacy chart customization metadata was not normalized before scope calculation, so some charts that should have been in scope were skipped and never updated.
This PR fixes that by:
groupbyWhy this works:
groupby, which is what affected charts expectfiltersclause is no longer generatedThis keeps the fix focused on the reported dashboard Dynamic Group By issue.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
Selecting a Dynamic Group By value could fail to change the chart grouping because the selected column was incorrectly turned into a row filter, or because the chart was skipped during scope resolution.
AFTER
The same interaction now updates the chart query using the selected column in
groupby, without generating a bogus row filter, and scoped charts update correctly.TESTING INSTRUCTIONS
groupbyand add it to a dashboard.Apply filters.groupbyfiltersclause is generated using the selected column name as a valueAutomated:
npm run test -- src/dashboard/util/getFormDataWithExtraFilters.test.tsnpm run test -- src/dashboard/components/DashboardBuilder/DashboardContainer.test.tsxADDITIONAL INFORMATION