Copilot commented on code in PR #35867:
URL: https://github.com/apache/superset/pull/35867#discussion_r2470450485
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterControl/index.jsx:
##########
@@ -285,14 +285,20 @@ class AdhocFilterControl extends Component {
}
moveLabel(dragIndex, hoverIndex) {
- const { values } = this.state;
- const newValues = [...values];
- [newValues[hoverIndex], newValues[dragIndex]] = [
- newValues[dragIndex],
- newValues[hoverIndex],
- ];
- this.setState({ values: newValues });
+ const { values } = this.state;
+ if (
+ dragIndex === hoverIndex ||
+ dragIndex < 0 ||
+ hoverIndex < 0 ||
+ dragIndex >= values.length ||
+ hoverIndex >= values.length
+ ) {
+ return;
+ }
+ const [moved] = this.values.splice(dragIndex, 1);
+ this.values.splice(hoverIndex, 0, moved);
+ this.props.onChange(this.state.values);
Review Comment:
Bug: `this.values` is undefined. The code should use the local `values`
variable that was extracted from state on line 289. Additionally, the state
needs to be updated via `setState` before calling `onChange`.
Suggested fix:
```javascript
const newValues = [...values];
const [moved] = newValues.splice(dragIndex, 1);
newValues.splice(hoverIndex, 0, moved);
this.setState({ values: newValues }, () => {
this.props.onChange(newValues);
});
```
```suggestion
const newValues = [...values];
const [moved] = newValues.splice(dragIndex, 1);
newValues.splice(hoverIndex, 0, moved);
this.setState({ values: newValues }, () => {
this.props.onChange(newValues);
});
```
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx:
##########
@@ -96,7 +96,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
const onShiftOptions = useCallback(
(dragIndex: number, hoverIndex: number) => {
Review Comment:
The `reorder` method is called without bounds validation, unlike the similar
implementations in `DndMetricSelect.tsx` and `DndFilterSelect.tsx`. Consider
adding bounds checking before calling `reorder` for consistency and safety:
```typescript
if (
dragIndex === hoverIndex ||
dragIndex < 0 ||
hoverIndex < 0 ||
dragIndex >= optionSelector.values.length ||
hoverIndex >= optionSelector.values.length
) {
return;
}
```
```suggestion
(dragIndex: number, hoverIndex: number) => {
if (
dragIndex === hoverIndex ||
dragIndex < 0 ||
hoverIndex < 0 ||
dragIndex >= optionSelector.values.length ||
hoverIndex >= optionSelector.values.length
) {
return;
}
```
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx:
##########
@@ -252,12 +252,19 @@ const DndFilterSelect = (props: DndFilterSelectProps) => {
const onShiftOptions = useCallback(
(dragIndex: number, hoverIndex: number) => {
- const newValues = [...values];
- [newValues[hoverIndex], newValues[dragIndex]] = [
- newValues[dragIndex],
- newValues[hoverIndex],
- ];
- setValues(newValues);
+ if (
+ dragIndex === hoverIndex ||
+ dragIndex < 0 ||
+ hoverIndex < 0 ||
+ dragIndex >= values.length ||
+ hoverIndex >= values.length
+ ) {
+ return;
+ }
+ const newValue = [...values];
+ const [moved] = newValue.splice(dragIndex, 1);
+ newValue.splice(hoverIndex, 0, moved);
+ setValues(newValue);
},
[values],
Review Comment:
Missing `onChange` call after reordering. Unlike `DndMetricSelect.tsx` which
calls both `setValue` and `onChange` in its `moveLabel` function, this
implementation only calls `setValues` without propagating the change to the
parent component. This means the reordered values won't be persisted.
Suggested fix:
```typescript
const newValue = [...values];
const [moved] = newValue.splice(dragIndex, 1);
newValue.splice(hoverIndex, 0, moved);
setValues(newValue);
onChange(newValue); // Add this line
```
```suggestion
onChange(newValue);
},
[onChange, values],
```
--
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]
