codeant-ai-for-open-source[bot] commented on code in PR #37528:
URL: https://github.com/apache/superset/pull/37528#discussion_r2737868272
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -119,7 +119,7 @@ const StyledContent = styled.div<{
}>`
grid-column: 2;
grid-row: 2;
- // @z-index-above-dashboard-header (100) + 1 = 101
+ /* @z-index-above-dashboard-header (100) + 1 = 101 */
${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`}
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ 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.
```
</details>
```suggestion
${({ fullSizeChartId }) =>
fullSizeChartId !== null && fullSizeChartId !== undefined
? `z-index: 101;`
: ''}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -473,9 +475,9 @@ const DashboardBuilder = () => {
() => ({
marginLeft:
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Top-level tabs layout shifts incorrectly.
- ⚠️ Dashboard header alignment affected in some modes.
- ⚠️ Affects drag/drop style in builder/header Droppable.
```
</details>
```suggestion
marginLeft: (
dashboardFiltersOpen ||
editMode ||
!nativeFiltersEnabled ||
filterBarOrientation === FilterBarOrientation.Horizontal
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
