codeant-ai-for-open-source[bot] commented on code in PR #36993:
URL: https://github.com/apache/superset/pull/36993#discussion_r2741366500
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -136,7 +142,7 @@ const renderWithRouter = ({
<ExploreViewContainer />
</Route>
</MemoryRouter>,
- { useRedux: true, useDnd: true, initialState },
+ { useRedux: true, useDnd: true, initialState, store },
);
Review Comment:
**Suggestion:** `renderWithRouter` overwrites `window.location` without
preserving or restoring the original value, which can leak state between tests
and break other browser APIs; save the original `window.location` and restore
it after calling `render`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ExploreViewContainer tests may become flaky due to leaked location.
- ⚠️ Tests asserting window.history.replaceState behave unpredictably.
- ⚠️ Cross-test interference inside the same test file.
```
</details>
```suggestion
const originalLocation = window.location;
Object.defineProperty(window, 'location', {
get() {
return { pathname: path, search };
},
configurable: true,
});
try {
return render(
<MemoryRouter initialEntries={[`${path}${search}`]}>
<Route path={path}>
<ExploreViewContainer />
</Route>
</MemoryRouter>,
{ useRedux: true, useDnd: true, initialState, store },
);
} finally {
// restore original location to avoid leaking mocked location into other
tests
try {
Object.defineProperty(window, 'location', {
value: originalLocation,
configurable: true,
});
} catch (e) {
// best-effort restore; if restore fails don't throw from test helper
}
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open file
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx
and locate renderWithRouter definition at lines 132-146. The helper sets
window.location
via Object.defineProperty(window, 'location', ...) (lines 134-138) and then
calls
render(...).
2. Run the jest test file (e.g., yarn test
src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx).
The first test
that calls renderWithRouter is `test('generates a new form_data param when
none is
available', ...)` (starts at line ~149). renderWithRouter will set
window.location to {
pathname, search } and return without restoring the original window.location.
3. A later test in the same file, such as `test('doesnt call replaceState
when pathname is
not /explore', ...)` (appears after the previous tests), expects
window.location to be
readable or manipulated in a specific way (it spies on
window.history.replaceState).
Because window.location was redefined and not restored, this can change the
value seen by
subsequent tests and cause unexpected behavior or flakiness.
4. Observe flaky failures or unexpected assertions in tests that read or use
window.location/window.history across multiple renderWithRouter usages in
this file.
Restoring the original window.location after renderWithRouter (as in the
improved_code)
prevents the mocked location from leaking between tests. If the test helper
intentionally
isolates location for an individual test, the improved code preserves that
isolation and
then restores the global to avoid cross-test pollution.
```
</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/ExploreViewContainer/ExploreViewContainer.test.tsx
**Line:** 134:146
**Comment:**
*Logic Error: `renderWithRouter` overwrites `window.location` without
preserving or restoring the original value, which can leak state between tests
and break other browser APIs; save the original `window.location` and restore
it after calling `render`.
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/ExploreViewContainer/index.jsx:
##########
@@ -571,6 +571,29 @@ function ExploreViewContainer(props) {
}
}
+ // Automatically set axis title margins when titles are added or removed
+ if (changedControlKeys.includes('x_axis_title')) {
+ const xAxisTitle = props.controls.x_axis_title?.value || '';
+ const currentMargin = props.controls.x_axis_title_margin?.value ?? 0;
+
+ if (xAxisTitle && currentMargin < 30) {
+ props.actions.setControlValue('x_axis_title_margin', 30);
+ } else if (!xAxisTitle && currentMargin !== 0) {
+ props.actions.setControlValue('x_axis_title_margin', 0);
+ }
+ }
+
+ if (changedControlKeys.includes('y_axis_title')) {
+ const yAxisTitle = props.controls.y_axis_title?.value || '';
+ const currentMargin = props.controls.y_axis_title_margin?.value ?? 0;
+
Review Comment:
**Suggestion:** Logic error: the code uses `currentMargin < 30` to decide
when to auto-set the margin to 30, which will overwrite any smaller
user-customized non-zero margins (e.g., 10 or 20). Per the intended behavior,
the margin should only be auto-set when it is exactly 0 (the default), not
whenever it is less than 30. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Overrides user axis-margin preferences in Explore controls.
- ❌ X axis title margin unexpectedly changes when edited.
- ❌ Y axis title margin unexpectedly changes when edited.
- ⚠️ Affects chart editing UX in ExploreViewContainer.
```
</details>
```suggestion
if (xAxisTitle && currentMargin === 0) {
props.actions.setControlValue('x_axis_title_margin', 30);
} else if (!xAxisTitle && currentMargin !== 0) {
props.actions.setControlValue('x_axis_title_margin', 0);
}
}
if (changedControlKeys.includes('y_axis_title')) {
const yAxisTitle = props.controls.y_axis_title?.value || '';
const currentMargin = props.controls.y_axis_title_margin?.value ?? 0;
if (yAxisTitle && currentMargin === 0) {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Explore view and load a chart editor (ExploreViewContainer at
superset-frontend/src/explore/components/ExploreViewContainer/index.jsx).
The controls
change handling runs in the useEffect that computes changedControlKeys and
contains the
axis-margin logic (new logic is located at lines 574-594 in the PR hunk).
2. In the Controls panel (ConnectedControlPanelsContainer rendered in
ExploreViewContainer), set a non-zero custom margin for the X axis title
control (the
control key is x_axis_title_margin). This writes a non-zero value (e.g., 10)
into
props.controls.x_axis_title_margin.value.
3. Add or change the X axis title (update x_axis_title). The change causes
props.controls
to update; the useEffect detects changedControlKeys includes 'x_axis_title'
and executes
the block at lines 575-583. The code reads currentMargin and evaluates
`currentMargin <
30` at line 579.
4. Because the condition uses `< 30`, the earlier user-chosen margin (10)
satisfies the
condition and is overwritten by
props.actions.setControlValue('x_axis_title_margin', 30)
at line 580. The user's deliberate smaller margin is lost. (Same flow for
the Y axis with
lines 586-593.)
Note: This reproduces the exact behavior in the added block (lines 574-594).
The change
from `< 30` to `=== 0` prevents overwriting user-custom values and only
auto-sets when
margin is exactly the default 0.
```
</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/ExploreViewContainer/index.jsx
**Line:** 579:589
**Comment:**
*Logic Error: Logic error: the code uses `currentMargin < 30` to decide
when to auto-set the margin to 30, which will overwrite any smaller
user-customized non-zero margins (e.g., 10 or 20). Per the intended behavior,
the margin should only be auto-set when it is exactly 0 (the default), not
whenever it is less than 30.
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]
