feat: Add explicit box-sizing to chart holders for consistent embedde… #37528
Conversation
…d dashboard rendering and update package-lock.json.
Code Review Agent Run #4bc508Actionable 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 |
dosubot
bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| grid-row: 2; | ||
| // @z-index-above-dashboard-header (100) + 1 = 101 | ||
| /* @z-index-above-dashboard-header (100) + 1 = 101 */ | ||
| ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`} |
There was a problem hiding this comment.
Suggestion: Bug: fullSizeChartId is tested with a truthy check. If fullSizeChartId is 0 (a valid chart id), the condition is false and the z-index is not applied; this will cause the intended stacking behavior to fail for charts with id 0. Change the check to explicitly test for null/undefined (or test the type) instead of truthiness. [logic error]
Severity Level: Major ⚠️
- ❌ Full-size chart overlay stacking incorrect when id is 0.
- ⚠️ Dashboard full-screen display may overlap other elements.
- ⚠️ Affects embedded and native dashboard rendering consistency.| ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`} | |
| ${({ fullSizeChartId }) => | |
| fullSizeChartId !== null && fullSizeChartId !== undefined | |
| ? `z-index: 101;` | |
| : ''} |
Steps of Reproduction ✅
1. Open the code file
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx and
locate the selector that reads the full-size chart id: the component reads Redux state via
the selector (const fullSizeChartId = useSelector<RootState, number | null>( state =>
state.dashboardState.fullSizeChartId );). This selector is in the DashboardBuilder
component (the selector is defined in this same file before the UI rendering).
2. Cause the Redux value dashboardState.fullSizeChartId to be set to 0. This can be
reproduced in practice by triggering the code-path that toggles a chart to "full-size" for
a chart whose id is 0 (the DashboardBuilder reads that state and passes it into the styled
container). Observe that DashboardBuilder uses that value in the styled component at lines
117-124 (StyledContent definition).
3. When fullSizeChartId is 0, the existing expression at lines 117-124 uses a truthy check
("${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`}") which evaluates 0 as
falsy; the z-index is not emitted into the rendered CSS. This is visible by inspecting the
DOM/CSS after the Redux state update: the StyledContent element will not have the intended
z-index.
4. Result: the full-size chart overlay/stacking behavior that relies on z-index 101 does
not apply when the selected chart id equals 0, causing the chart overlay to be stacked
incorrectly under other elements. The correct fix is to explicitly test for null/undefined
as shown in the suggested improved code. Note: this is not a stylistic change — the
current pattern treats numeric id 0 as falsy and is therefore an actual logic bug in cases
where a chart id of 0 is possible.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
**Line:** 123:123
**Comment:**
*Logic Error: Bug: `fullSizeChartId` is tested with a truthy check. If `fullSizeChartId` is 0 (a valid chart id), the condition is false and the z-index is not applied; this will cause the intended stacking behavior to fail for charts with id 0. Change the check to explicitly test for null/undefined (or test the type) instead of truthiness.
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.| @@ -473,9 +475,9 @@ const DashboardBuilder = () => { | |||
| () => ({ | |||
| marginLeft: | |||
There was a problem hiding this comment.
Suggestion: Bug: the ternary operator that selects marginLeft may bind to the wrong operand due to operator-precedence ambiguity with multiple || expressions, producing unexpected results; wrap the OR condition in parentheses so the combined boolean is the ternary condition. [possible bug]
Severity Level: Major ⚠️
- ❌ Top-level tabs layout shifts incorrectly.
- ⚠️ Dashboard header alignment affected in some modes.
- ⚠️ Affects drag/drop style in builder/header Droppable.| marginLeft: | |
| marginLeft: ( | |
| dashboardFiltersOpen || | |
| editMode || | |
| !nativeFiltersEnabled || | |
| filterBarOrientation === FilterBarOrientation.Horizontal | |
| ) |
Steps of Reproduction ✅
1. Open superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
and locate the draggableStyle definition at lines 474-483 (const draggableStyle =
useMemo(...)). The returned object is later passed as the style prop to the Droppable
component in the header (see the Droppable usage that contains style={draggableStyle} in
the same file).
2. Load any dashboard in the application so the DashboardBuilder is rendered and the
Droppable in the header is mounted (the Droppable is within the StyledHeader block that
renders top-level tabs). Observe the computed style for the Droppable element (via browser
devtools).
3. Toggle the boolean inputs that feed into the condition: e.g., set
dashboardFiltersOpen=false, editMode=true, nativeFiltersEnabled=true, and
filterBarOrientation to a non-horizontal value. Without parentheses the JavaScript
conditional operator may be parsed in an unintended way (operator precedence between ||
and ? : can lead to "a || (b ? c : d)" instead of "(a || b) ? c : d"), causing marginLeft
to evaluate to the wrong branch. The problematic expression is at lines 474-483.
4. Result: the Droppable (top-level tabs wrapper) receives an incorrect marginLeft (0 vs
-32), which changes header/layout positioning. Reproducing this with the exact boolean
combinations above will demonstrate the incorrect layout; adding parentheses around the OR
chain (as in the improved code) ensures the combined boolean is used as the ternary
condition and fixes the layout inconsistency.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx
**Line:** 476:476
**Comment:**
*Possible Bug: Bug: the ternary operator that selects marginLeft may bind to the wrong operand due to operator-precedence ambiguity with multiple `||` expressions, producing unexpected results; wrap the OR condition in parentheses so the combined boolean is the ternary condition.
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.
|
Can you add some before/after screenshots? I think we saw another PR about this, but I like that this seems more focused. There are also some bot comments that you can address/dismiss. |
rusackas
added
the
review:checkpoint
There was a problem hiding this comment.
Pull request overview
This PR fixes a visual rendering issue in embedded dashboards where chart panels overlap due to inconsistent box-sizing behavior. The issue occurs because embedded dashboards lack the .ant-layout wrapper that provides global box-sizing: border-box styling, causing the .dashboard-component-chart-holder element to default to content-box and add padding outside its calculated width.
Changes:
- Added explicit
box-sizing: border-boxto.dashboard-component-chart-holderCSS class to ensure consistent rendering in both native and embedded dashboard contexts - Standardized comment syntax from
//to/* */style within styled-components for consistency - Minor whitespace/formatting adjustments for code readability
| /* Explicit box-sizing ensures consistent behavior in embedded mode, | ||
| where the global .ant-layout wrapper providing border-box is absent */ | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
The PR description incorrectly claims to include a database migration. All checkboxes under "ADDITIONAL INFORMATION" related to database migrations are checked (including "Includes DB Migration", "Migration is atomic", "Confirm DB migration upgrade and downgrade tested", and "Runtime estimates and downtime expectations provided"), but this PR only contains CSS/styling changes to a TypeScript React component. No database migration files are included or modified in this PR.
Please uncheck all the database migration related checkboxes in the PR description as they do not apply to this change.
2 participants
fix(dashboard): ensure consistent box-sizing for embedded dashboard rendering
SUMMARY
This PR fixes a panel overlap issue in embedded dashboards when using
@superset-ui/embedded-sdk.Problem:
The
.dashboard-component-chart-holderclass usespadding: 16px, but relies on the globalbox-sizing: border-boxstyle provided by the.ant-layoutwrapper. In embedded dashboards, this wrapper is absent, causing the element to default tobox-sizing: content-box. This adds 32px (16px × 2) to each panel's calculated width, making adjacent panels overlap/encroach on each other.Solution:
Added explicit
box-sizing: border-boxto.dashboard-component-chart-holderso the padding is included within the element's width, ensuring consistent rendering in both native and embedded dashboard views.Files changed:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before (embedded mode): Panels overlap due to content-box sizing adding padding outside width
After (embedded mode): Panels display correctly side-by-side with border-box sizing
TESTING INSTRUCTIONS
@superset-ui/embedded-sdkADDITIONAL INFORMATION