codeant-ai-for-open-source[bot] commented on code in PR #37501:
URL: https://github.com/apache/superset/pull/37501#discussion_r2739880208
##########
superset-frontend/src/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts:
##########
@@ -16,30 +16,35 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { useState, useCallback, useRef } from 'react';
+import { useMemo } from 'react';
import { useResizeDetector } from 'react-resize-detector';
export default function useResizeDetectorByObserver() {
- const ref = useRef<HTMLDivElement>(null);
- const [{ width, height }, setChartPanelSize] = useState<{
- width?: number;
- height?: number;
- }>({});
- const onResize = useCallback(() => {
- if (ref.current) {
- const { width, height } = ref.current.getBoundingClientRect?.() || {};
- setChartPanelSize({ width, height });
- }
- }, []);
- const { ref: observerRef } = useResizeDetector({
+ const {
+ width: rawWidth,
+ height: rawHeight,
+ ref,
+ } = useResizeDetector({
refreshMode: 'debounce',
- refreshRate: 300,
- onResize,
+ refreshRate: 100,
+ handleHeight: true,
+ handleWidth: true,
+ skipOnMount: false,
});
+ // Round dimensions immediately to prevent sub-pixel render loops
+ const width = useMemo(
+ () => (rawWidth ? Math.floor(rawWidth) : undefined),
Review Comment:
**Suggestion:** Falsy-value bug: the check `rawWidth ? ... : undefined`
treats 0 (and other falsy numeric values like NaN) as absent and returns
`undefined` instead of 0, causing consumers to believe there's no width when
the element is actually 0px wide; replace the truthy check with a finite-number
check (e.g., `rawWidth != null && Number.isFinite(rawWidth)`). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ExploreChartPanel charts receive incorrect width value.
- ⚠️ Chart rendering falls back to stale dimensions.
- ⚠️ UI overlap/visual glitches when panels collapse.
```
</details>
```suggestion
() => (rawWidth != null && Number.isFinite(rawWidth) ?
Math.floor(rawWidth) : undefined),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the frontend with the PR code and open the Explore page that mounts
the chart
panel.
2. The hook `useResizeDetectorByObserver()` defined in
`superset-frontend/src/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts`
collects raw sizes (see rawWidth at line 24 in this file). When the chart
container
becomes exactly 0px wide (for example, by collapsing adjacent UI panels),
the resize
observer sets `rawWidth` to 0 at the assignment on line 24.
3. The width memoization at lines 36-39 executes: `() => (rawWidth ?
Math.floor(rawWidth)
: undefined)`. Because `0` is falsy, the expression returns `undefined`
instead of `0`.
4. Any consumer of this hook (the Explore chart panel component that imports
this hook)
receives `width === undefined` instead of `width === 0`. Consumers that
treat undefined as
"not measured" may skip rendering or reuse previous dimensions, causing
visible
layout/rendering mismatches (overlap) when the container is actually 0px
wide.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts
**Line:** 37:37
**Comment:**
*Logic Error: Falsy-value bug: the check `rawWidth ? ... : undefined`
treats 0 (and other falsy numeric values like NaN) as absent and returns
`undefined` instead of 0, causing consumers to believe there's no width when
the element is actually 0px wide; replace the truthy check with a finite-number
check (e.g., `rawWidth != null && Number.isFinite(rawWidth)`).
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/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts:
##########
@@ -16,30 +16,35 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { useState, useCallback, useRef } from 'react';
+import { useMemo } from 'react';
import { useResizeDetector } from 'react-resize-detector';
export default function useResizeDetectorByObserver() {
- const ref = useRef<HTMLDivElement>(null);
- const [{ width, height }, setChartPanelSize] = useState<{
- width?: number;
- height?: number;
- }>({});
- const onResize = useCallback(() => {
- if (ref.current) {
- const { width, height } = ref.current.getBoundingClientRect?.() || {};
- setChartPanelSize({ width, height });
- }
- }, []);
- const { ref: observerRef } = useResizeDetector({
+ const {
+ width: rawWidth,
+ height: rawHeight,
+ ref,
+ } = useResizeDetector({
refreshMode: 'debounce',
- refreshRate: 300,
- onResize,
+ refreshRate: 100,
+ handleHeight: true,
+ handleWidth: true,
+ skipOnMount: false,
});
+ // Round dimensions immediately to prevent sub-pixel render loops
+ const width = useMemo(
+ () => (rawWidth ? Math.floor(rawWidth) : undefined),
+ [rawWidth],
+ );
+
+ const height = useMemo(
+ () => (rawHeight ? Math.floor(rawHeight) : undefined),
Review Comment:
**Suggestion:** Falsy-value bug for height: the check `rawHeight ? ... :
undefined` treats 0 (and other non-finite values) as absent and yields
`undefined` instead of the actual 0 height; guard with a finite-number check
(`rawHeight != null && Number.isFinite(rawHeight)`). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ExploreChartPanel charts receive incorrect height value.
- ⚠️ Chart layout falls back to stale height measurements.
- ⚠️ Visual overlap when container is collapsed to zero height.
```
</details>
```suggestion
() => (rawHeight != null && Number.isFinite(rawHeight) ?
Math.floor(rawHeight) : undefined),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the app and open the Explore page so the chart panel mounts the hook
`useResizeDetectorByObserver()` (this file).
2. Cause the chart container to have height `0` (for example, collapse a
vertical toolbar
or use a browser devtools layout adjustment). `rawHeight` is assigned at
line 25 in this
file by the resize detector.
3. The height memo at lines 41-44 runs `() => (rawHeight ?
Math.floor(rawHeight) :
undefined)`; because `rawHeight === 0` is falsy, the result becomes
`undefined` instead of
`0`.
4. Components reading `height` may treat `undefined` as "not measured" and
skip layout
updates or reuse previous heights, producing rendering/overlap issues in
Explore charts
when the actual container height is zero.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts
**Line:** 42:42
**Comment:**
*Logic Error: Falsy-value bug for height: the check `rawHeight ? ... :
undefined` treats 0 (and other non-finite values) as absent and yields
`undefined` instead of the actual 0 height; guard with a finite-number check
(`rawHeight != null && Number.isFinite(rawHeight)`).
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/explore/components/ExploreChartPanel/index.tsx:
##########
@@ -243,19 +262,23 @@ const ExploreChartPanel = ({
setShowSplit(isOpen);
}, []);
- const renderChart = useCallback(
+ const renderChart = useMemo(
() => (
<div
css={css`
min-height: 0;
- flex: 1;
- overflow: auto;
+ flex: 1 1 auto;
+ overflow: hidden;
+ position: relative;
+ box-sizing: border-box;
+ opacity: ${chartPanelWidth && chartPanelHeight ? 1 : 0};
Review Comment:
**Suggestion:** The opacity check uses truthiness (`chartPanelWidth &&
chartPanelHeight`) which treats 0 as falsy; if a dimension is legitimately 0
the element will be hidden. Use explicit undefined/null checks so zero
dimensions don't incorrectly force opacity 0. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Explore chart panel can remain hidden unexpectedly.
- ⚠️ ChartContainer not mounted preventing chart render.
- ⚠️ Standalone/hidden-mount scenarios show blank charts.
```
</details>
```suggestion
opacity: ${typeof chartPanelWidth !== 'undefined' && typeof
chartPanelHeight !== 'undefined' ? 1 : 0};
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Mount the Explore chart editor which instantiates ExploreChartPanel
(component defined
at src/explore/components/ExploreChartPanel/index.tsx, around the function
definition at
the top of the file). The renderChart block is created by the useMemo
beginning at the
hunk shown around lines 265-276 in the PR diff.
2. The hook useResizeDetectorByObserver
(src/explore/components/ExploreChartPanel/useResizeDetectorByObserver.ts,
lines 22-51)
returns a width/height computed via Math.floor(rawWidth/rawHeight). If the
DOM measurement
returns a small value that floors to 0 (rawWidth in [0,1)),
useResizeDetectorByObserver
sets width = 0 (concrete code: Math.floor(rawWidth) at
src/.../useResizeDetectorByObserver.ts:30-38).
3. In the renderChart CSS
(src/explore/components/ExploreChartPanel/index.tsx:269-276 per
the PR hunk), the opacity is computed with the truthy expression at line
274: opacity:
${chartPanelWidth && chartPanelHeight ? 1 : 0};. When chartPanelWidth === 0
(a real value
returned by the resize hook), this expression evaluates falsy causing
opacity to be 0.
4. Additionally, ChartContainer is conditionally rendered only when the same
truthy
expression is true (see the conditional at
src/explore/components/ExploreChartPanel/index.tsx:279-283 in the hunk).
Therefore a
measured dimension of 0 prevents ChartContainer from mounting and the chart
area remains
hidden/transparent in the Explore panel until dimensions change to non-zero.
Note: This is reproducible when the component is measured with a very small
size on mount
(e.g., mounted into a hidden or collapsed DOM container), which is exactly
when
useResizeDetectorByObserver can return 0 due to Math.floor; the fix changes
the check to
explicit undefined checks so legitimate numeric zeros are treated as valid
dimensions.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
**Line:** 274:274
**Comment:**
*Logic Error: The opacity check uses truthiness (`chartPanelWidth &&
chartPanelHeight`) which treats 0 as falsy; if a dimension is legitimately 0
the element will be hidden. Use explicit undefined/null checks so zero
dimensions don't incorrectly force opacity 0.
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]
