codeant-ai-for-open-source[bot] commented on code in PR #38307:
URL: https://github.com/apache/superset/pull/38307#discussion_r3465722230
##########
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx:
##########
@@ -379,6 +379,79 @@ test('should fallback to formData state when runtime state
not available', () =>
expect(getByTestId('chart-container')).toBeInTheDocument();
});
+test('chart height is reduced on first render in expanded state (guards
against useEffect regression)', () => {
+ const DESCRIPTION_HEIGHT = 60;
+ const CHART_HEIGHT = 300;
+ // Matches the DEFAULT_HEADER_HEIGHT constant in Chart.tsx.
+ const DEFAULT_HEADER_HEIGHT = 22;
+
+ // Stabilise getHeaderHeight(): emotion injects margin-bottom CSS during
+ // React's commit phase, so getComputedStyle returns different values in
+ // initial renders vs re-renders. Mock it to always return empty so
+ // getHeaderHeight() consistently falls back to DEFAULT_HEADER_HEIGHT.
+ const getComputedStyleSpy = jest
+ .spyOn(window, 'getComputedStyle')
+ .mockReturnValue({
+ getPropertyValue: () => '',
+ } as unknown as CSSStyleDeclaration);
+
+ // JSDOM doesn't compute layout, so mock offsetHeight to simulate a real
+ // description element with height.
+ const offsetHeightSpy = jest
+ .spyOn(HTMLElement.prototype, 'offsetHeight', 'get')
+ .mockImplementation(function (this: HTMLElement) {
+ return this.classList.contains('slice_description')
+ ? DESCRIPTION_HEIGHT
+ : 0;
+ });
+
+ // Suppress all passive effects to simulate the first-paint moment — the
+ // point at which the original useEffect bug caused clipping. useLayoutEffect
+ // (the fix) runs synchronously before paint and is intentionally NOT mocked
+ // here. If the implementation were reverted to useEffect, this spy would
+ // prevent the height measurement and the assertion below would fail.
+ const useEffectSpy = jest
+ .spyOn(global.React, 'useEffect')
+ .mockImplementation(() => {});
+
+ const { container } = setup(
+ { height: CHART_HEIGHT },
+ {
+ charts: {
+ ...defaultState.charts,
+ [queryId]: {
+ ...defaultState.charts[queryId],
+ // ChartOverlay renders with an inline height style when loading —
+ // this is the observable proxy for getChartHeight() without real
layout.
+ chartStatus: 'loading',
+ },
+ },
+ dashboardState: {
+ ...defaultState.dashboardState,
+ expandedSlices: { [queryId]: true },
+ },
+ },
+ );
+
+ const chartHeight = parseInt(
+ container.querySelector<HTMLDivElement>('.dashboard-chart > div[style]')!
+ .style.height,
+ 10,
+ );
+
+ // useLayoutEffect must have measured and applied descriptionHeight
+ // synchronously. If useEffect were used instead, descriptionHeight would
+ // still be 0 here (suppressed by useEffectSpy) and chartHeight would equal
+ // CHART_HEIGHT - DEFAULT_HEADER_HEIGHT rather than the value below.
+ expect(chartHeight).toBe(
+ CHART_HEIGHT - DEFAULT_HEADER_HEIGHT - DESCRIPTION_HEIGHT,
+ );
+
+ useEffectSpy.mockRestore();
+ getComputedStyleSpy.mockRestore();
+ offsetHeightSpy.mockRestore();
Review Comment:
**Suggestion:** The test installs global spies (`getComputedStyle`,
`offsetHeight`, and `React.useEffect`) and restores them only at the end of the
test body. If any assertion or render step throws before those restore calls,
the mocked globals leak into later tests and can cause cascading,
order-dependent failures. Wrap the body in `try/finally` (or move cleanup to
`afterEach` with `jest.restoreAllMocks`) so cleanup always runs. [missing
cleanup]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Chart.test suite can leak mocks across tests.
- ⚠️ Later tests may run with React.useEffect disabled.
- ⚠️ Test failures become cascading and harder to debug.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx`
and note the `afterEach` hook at lines 134-136, which calls only
`jest.clearAllMocks()`
and does not call `jest.restoreAllMocks()`.
2. In the test `"chart height is reduced on first render in expanded state
(guards against
useEffect regression)"` at lines 43-114, observe that it installs spies on
`window.getComputedStyle`, `HTMLElement.prototype.offsetHeight`, and
`global.React.useEffect` (lines 53-76) and only restores them at the end of
the test body
(lines 111-113).
3. Introduce an early failure in this test (for example, temporarily change
the
expectation at lines 107-109 so it fails), then run this test file with Jest
(`jest
superset-frontend/src/dashboard/components/gridComponents/Chart/Chart.test.tsx`).
The test
throws before executing the `useEffectSpy.mockRestore()`,
`getComputedStyleSpy.mockRestore()`, and `offsetHeightSpy.mockRestore()`
calls.
4. After the failing test, Jest executes the `afterEach` hook (line 134),
which clears
mock call history but does not restore the original implementations; the
spies on
`window.getComputedStyle`, `HTMLElement.prototype.offsetHeight`, and
`global.React.useEffect` remain active. Subsequent tests in this file, such
as `"should
not show a close button on chart error banners"` at lines 116-139, now run
with these
globals still mocked, leading to order-dependent, hard-to-diagnose test
failures if they
rely on the real implementations.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=734b73fe3b4a4b55898454c972b68285&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=734b73fe3b4a4b55898454c972b68285&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/gridComponents/Chart/Chart.test.tsx
**Line:** 392:452
**Comment:**
*Missing Cleanup: The test installs global spies (`getComputedStyle`,
`offsetHeight`, and `React.useEffect`) and restores them only at the end of the
test body. If any assertion or render step throws before those restore calls,
the mocked globals leak into later tests and can cause cascading,
order-dependent failures. Wrap the body in `try/finally` (or move cleanup to
`afterEach` with `jest.restoreAllMocks`) so cleanup always runs.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38307&comment_hash=1e4309834664822814a56ceb504b354ac0b4318ed150df768fc8ebcd46a0021f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38307&comment_hash=1e4309834664822814a56ceb504b354ac0b4318ed150df768fc8ebcd46a0021f&reaction=dislike'>👎</a>
--
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]
