korbit-ai[bot] commented on code in PR #33170:
URL: https://github.com/apache/superset/pull/33170#discussion_r2056527477
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts:
##########
@@ -362,9 +368,23 @@ export function saveNativeFilterSettings(charts:
ChartSpec[]) {
cy.get(nativeFilters.modal.footer)
.contains('Save')
.should('be.visible')
- .click();
- cy.get(nativeFilters.modal.container).should('not.exist');
- charts.forEach(waitForChartLoad);
+ .click({ force: true });
+
+ cy.wait(2000);
+
+ cy.get('body').then($body => {
+ if ($body.find(nativeFilters.modal.container).length > 0) {
+ cy.get(nativeFilters.modal.footer)
+ .contains('Save')
+ .click({ force: true });
+
+ cy.wait(1000);
+ }
Review Comment:
### Unreliable Modal Save Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The saveNativeFilterSettings function uses arbitrary wait times (2000ms and
1000ms) and a retry mechanism to handle modal closure, which is fragile and
could lead to intermittent test failures.
###### Why this matters
Using fixed wait times can cause tests to fail when network conditions vary
or when the application behaves slower/faster than expected. This could result
in flaky tests that sometimes pass and sometimes fail.
###### Suggested change ∙ *Feature Preview*
Replace the wait-based approach with proper assertions and waiters:
```typescript
export function saveNativeFilterSettings(charts: ChartSpec[]) {
cy.get(nativeFilters.modal.footer)
.contains('Save')
.should('be.visible')
.click({ force: true });
// Wait for modal to either close or remain open
cy.get('body').should(($body) => {
const modalExists = $body.find(nativeFilters.modal.container).length > 0;
if (modalExists) {
cy.get(nativeFilters.modal.footer)
.contains('Save')
.should('be.visible')
.click({ force: true });
}
});
// Ensure modal is closed
cy.get(nativeFilters.modal.container).should('not.exist');
// Wait for all charts to load
charts.forEach(chart => {
waitForChartLoad(chart);
});
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95c8a72f-b562-4194-a64b-8119e2549a9a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95c8a72f-b562-4194-a64b-8119e2549a9a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95c8a72f-b562-4194-a64b-8119e2549a9a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95c8a72f-b562-4194-a64b-8119e2549a9a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/95c8a72f-b562-4194-a64b-8119e2549a9a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:32777029-03d1-4283-a21f-ac5e8405db80 -->
[](32777029-03d1-4283-a21f-ac5e8405db80)
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -1250,9 +1322,20 @@ const FiltersConfigForm = (
rules={[
{
validator: () => {
- if (formFilter?.defaultDataMask?.filterState?.value)
{
- // requires managing the error as the DefaultValue
- // component does not use an Antdesign compatible
input
+ // For range filters, check if at least one of the
values in the array is non-null
+ const value =
+ formFilter?.defaultDataMask?.filterState?.value;
+ const isRangeFilter =
+ formFilter?.filterType === 'filter_range';
+
+ // Check if value exists and is valid
+ const hasValidValue =
+ (isRangeFilter &&
+ Array.isArray(value) &&
+ (value[0] !== null || value[1] !== null)) ||
+ (!isRangeFilter && !!value);
Review Comment:
### Complex Validation Logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Complex boolean logic embedded directly in the validator makes the
validation rules hard to understand at a glance.
###### Why this matters
Dense boolean expressions reduce code maintainability and make it harder to
modify validation rules in the future.
###### Suggested change ∙ *Feature Preview*
Extract into a clear validator function:
```typescript
const isValidFilterValue = (value: any, isRangeFilter: boolean) => {
if (isRangeFilter) {
return Array.isArray(value) && (value[0] !== null || value[1] !== null);
}
return !!value;
};
const hasValidValue = isValidFilterValue(value, isRangeFilter);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75c810a8-eebd-4394-b028-533a2b38f906/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75c810a8-eebd-4394-b028-533a2b38f906?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75c810a8-eebd-4394-b028-533a2b38f906?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75c810a8-eebd-4394-b028-533a2b38f906?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/75c810a8-eebd-4394-b028-533a2b38f906)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7bd4dc26-89cd-4afb-8277-5bee8788d5fa -->
[](7bd4dc26-89cd-4afb-8277-5bee8788d5fa)
##########
superset-frontend/cypress-base/cypress/e2e/dashboard/utils.ts:
##########
@@ -293,16 +293,22 @@
}
cy.get(nativeFilters.silentLoading).should('not.exist');
if (filterColumn) {
- cy.get(nativeFilters.filtersPanel.filterInfoInput)
- .last()
+ cy.contains('label', 'Column').closest('.ant-form-item').as('columnField');
+
+ cy.get('@columnField').find('.ant-select-selector').click({ force: true });
+
+ cy.wait(300);
+
+ cy.get('.ant-select-selection-search-input:focus').type(filterColumn, {
+ force: true,
+ });
+
+ cy.wait(500);
Review Comment:
### Replace sequential waits with clear state expectations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Multiple arbitrary wait times used in succession for form interaction,
making the code's intent unclear and timing-dependent.
###### Why this matters
Using sequential arbitrary waits obscures what state changes or UI updates
we're actually waiting for, making the code fragile and hard to maintain.
###### Suggested change ∙ *Feature Preview*
```typescript
// Wait for the input to be ready and visible
cy.get('.ant-select-selection-search-input')
.should('be.visible')
.and('not.be.disabled')
.type(filterColumn, { force: true });
// Wait for dropdown to be populated
cy.get('.ant-select-dropdown')
.should('be.visible')
.and('contain', filterColumn);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b1364072-2410-4b27-88a8-6183397410dc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b1364072-2410-4b27-88a8-6183397410dc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b1364072-2410-4b27-88a8-6183397410dc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b1364072-2410-4b27-88a8-6183397410dc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b1364072-2410-4b27-88a8-6183397410dc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ed8815d1-f7d7-40fd-a189-6c53385bbe14 -->
[](ed8815d1-f7d7-40fd-a189-6c53385bbe14)
##########
superset-frontend/src/dashboard/components/nativeFilters/utils.ts:
##########
@@ -83,6 +83,7 @@ export const getFormData = ({
return {
...controlValues,
...otherProps,
+ controlValues,
adhoc_filters: adhoc_filters ?? [],
Review Comment:
### Redundant Data Structure <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The getFormData function spreads controlValues and then redundantly includes
it again as a nested property, violating the DRY principle and creating
potential confusion.
###### Why this matters
This creates ambiguity in the data structure where the same values exist at
both the root level and nested level, making it harder to maintain and
potentially causing issues if the values need to be updated or accessed
consistently.
###### Suggested change ∙ *Feature Preview*
Remove the redundant controlValues property since the values are already
spread at the root level:
```typescript
return {
...controlValues,
...otherProps,
adhoc_filters: adhoc_filters ?? [],
// ... rest of the properties
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3ac46e86-d4a3-491f-b98c-66854704989a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3ac46e86-d4a3-491f-b98c-66854704989a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3ac46e86-d4a3-491f-b98c-66854704989a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3ac46e86-d4a3-491f-b98c-66854704989a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3ac46e86-d4a3-491f-b98c-66854704989a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9ef610f0-8a39-4545-951a-023500e24b40 -->
[](9ef610f0-8a39-4545-951a-023500e24b40)
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -303,29 +397,105 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
setError(null);
updateDataMaskValue(newInputValue);
},
- [col, min, max, enableEmptyFilter, enableSingleValue, setDataMask],
+ [
+ min,
+ max,
+ enableEmptyFilter,
+ enableSingleValue,
+ updateDataMaskError,
+ updateDataMaskValue,
+ inputValue,
+ ],
+ );
+
+ // Handler for slider change
+ const handleSliderChange = useCallback(
+ (value: number[]) => {
+ if (row?.min === undefined && row?.max === undefined) {
+ return;
+ }
Review Comment:
### Silent failure in input handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Silent return when row.min and row.max are undefined in handleChange,
potentially masking data integrity issues.
###### Why this matters
Silent failures make debugging difficult and could hide data loading or
transformation issues from users and developers.
###### Suggested change ∙ *Feature Preview*
```typescript
if (row?.min === undefined && row?.max === undefined) {
setError(t('Unable to load range values'));
updateDataMaskError(t('Unable to load range values'));
return;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef7eb2d2-4830-400e-abc5-fea7e62ba018/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef7eb2d2-4830-400e-abc5-fea7e62ba018?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef7eb2d2-4830-400e-abc5-fea7e62ba018?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef7eb2d2-4830-400e-abc5-fea7e62ba018?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ef7eb2d2-4830-400e-abc5-fea7e62ba018)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2b3b907a-930d-4bdf-86af-c4593bef5c49 -->
[](2b3b907a-930d-4bdf-86af-c4593bef5c49)
--
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]
