korbit-ai[bot] commented on code in PR #35867:
URL: https://github.com/apache/superset/pull/35867#discussion_r2468975453
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts:
##########
@@ -89,6 +89,11 @@ export class OptionSelector {
[this.values[a], this.values[b]] = [this.values[b], this.values[a]];
}
+ reorder(a: number, b: number) {
+ const [moved] = this.values.splice(a, 1);
+ this.values.splice(b, 0, moved);
+ }
Review Comment:
### Missing bounds validation in reorder method <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The reorder method lacks bounds checking for array indices, which can cause
runtime errors or undefined behavior when invalid indices are passed.
###### Why this matters
Without bounds validation, passing out-of-range indices could lead to
unexpected splice operations, potential runtime errors, or silent failures that
corrupt the array state.
###### Suggested change ∙ *Feature Preview*
Add bounds checking before performing splice operations:
```typescript
reorder(a: number, b: number) {
if (a < 0 || a >= this.values.length || b < 0 || b > this.values.length) {
return;
}
const [moved] = this.values.splice(a, 1);
this.values.splice(b, 0, moved);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58ee55f4-c091-468b-898f-1dd5cad9feac/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58ee55f4-c091-468b-898f-1dd5cad9feac?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58ee55f4-c091-468b-898f-1dd5cad9feac?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58ee55f4-c091-468b-898f-1dd5cad9feac?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58ee55f4-c091-468b-898f-1dd5cad9feac)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9a1b4e95-df21-4682-9670-4acf1e650a7b -->
[](9a1b4e95-df21-4682-9670-4acf1e650a7b)
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelect.tsx:
##########
@@ -96,7 +96,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
const onShiftOptions = useCallback(
(dragIndex: number, hoverIndex: number) => {
- optionSelector.swap(dragIndex, hoverIndex);
+ optionSelector.reorder(dragIndex, hoverIndex);
Review Comment:
### Unverified reorder method implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code calls `reorder` method on `optionSelector` but the implementation
and existence of this method cannot be verified from the provided code.
###### Why this matters
If the `reorder` method doesn't exist, has incorrect signature, or doesn't
implement proper bounds checking and immutable reordering as intended, the
drag-and-drop functionality will fail at runtime or continue to exhibit the
chaotic ordering behavior the change aims to fix.
###### Suggested change ∙ *Feature Preview*
Verify that the `OptionSelector` class in
`src/explore/components/controls/DndColumnSelectControl/utils` has a
`reorder(dragIndex: number, hoverIndex: number): void` method that implements
bounds-checked, immutable reordering as described in the developer intent.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31474633-71d6-463a-9052-56910247e6a2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31474633-71d6-463a-9052-56910247e6a2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31474633-71d6-463a-9052-56910247e6a2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31474633-71d6-463a-9052-56910247e6a2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/31474633-71d6-463a-9052-56910247e6a2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7df30bdc-8761-4c4e-aa7b-7ceda2a51650 -->
[](7df30bdc-8761-4c4e-aa7b-7ceda2a51650)
##########
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);
Review Comment:
### Undefined property reference in moveLabel <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code references `this.values` which is undefined, instead of
`this.state.values`.
###### Why this matters
This will cause a runtime error when attempting to reorder items via drag
and drop, as `this.values` does not exist on the component instance.
###### Suggested change ∙ *Feature Preview*
Replace `this.values` with `values` (the destructured state variable) or
create a proper copy:
```javascript
const newValues = [...values];
const [moved] = newValues.splice(dragIndex, 1);
newValues.splice(hoverIndex, 0, moved);
this.setState({ values: newValues });
this.props.onChange(newValues);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6998f229-d7ab-4410-955d-da0214d1c4f5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6998f229-d7ab-4410-955d-da0214d1c4f5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6998f229-d7ab-4410-955d-da0214d1c4f5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6998f229-d7ab-4410-955d-da0214d1c4f5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6998f229-d7ab-4410-955d-da0214d1c4f5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3c3c0967-6023-4e42-ba20-56bb17ed9959 -->
[](3c3c0967-6023-4e42-ba20-56bb17ed9959)
--
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]
