korbit-ai[bot] commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2173166552
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -466,6 +466,30 @@ function FiltersConfigModal({
handleErroredFilters();
+ // Validate dependencies to prevent saving cyclic dependencies
+ validateDependencies();
Review Comment:
### Incomplete Cyclic Dependency Validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The validateDependencies function is not preventing all cyclic dependencies
as it's only validating the existing dependencies and not potential ones that
could be created during the form editing process.
###### Why this matters
If a user is actively editing dependencies in the form, the validation may
not catch cycles that could be formed by these pending changes until after
they're committed to the form state.
###### Suggested change ∙ *Feature Preview*
Modify the validateDependencies function to include both existing and
pending dependencies:
```typescript
const validateDependencies = useCallback(() => {
const dependencyMap = buildDependencyMap();
const formValues = form.getFieldsValue().filters || {};
// Include pending changes from the form
Object.keys(formValues).forEach(key => {
const pendingDeps = formValues[key]?.dependencies || [];
dependencyMap.set(key, pendingDeps);
});
filterIds
.filter(id => !removedFilters[id])
.forEach(filterId => {
const result = hasCircularDependency(dependencyMap, filterId);
const field = {
name: ['filters', filterId, 'dependencies'] as ['filters',
string, 'dependencies'],
errors: result ? [t('Cyclic dependency detected')] : [],
};
form.setFields([field]);
});
handleErroredFilters();
}, [buildDependencyMap, filterIds, form, handleErroredFilters,
removedFilters]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3a9fb87-75c2-47de-96ae-ecf153f61703)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:283548ea-86ee-489c-a337-060bd648ce90 -->
[](283548ea-86ee-489c-a337-060bd648ce90)
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -466,6 +466,30 @@ function FiltersConfigModal({
handleErroredFilters();
+ // Validate dependencies to prevent saving cyclic dependencies
+ validateDependencies();
+
+ // Check if validation added any dependency errors
+ const fieldsWithErrors = form.getFieldsError();
+ const hasDependencyErrors = fieldsWithErrors.some(field =>
+ field.name?.[0] === 'filters' &&
+ field.name?.[2] === 'dependencies' &&
Review Comment:
### Magic Array Indices in Form Field Access <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic array indices [0] and [2] are used to access form field names without
type safety or clear meaning.
###### Why this matters
Using magic indices makes the code's intent unclear and brittle to changes
in the form field structure.
###### Suggested change ∙ *Feature Preview*
```typescript
interface FormField {
name: ['filters', string, 'dependencies'];
errors?: string[];
}
const isDependencyError = (field: FormField) =>
field.name[0] === 'filters' &&
field.name[2] === 'dependencies' &&
field.errors?.length > 0;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d899fe2b-7551-4d0c-bdcd-ab4a6aa77017)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d6edb9c7-daca-48e9-9642-8f6202eaf2ec -->
[](d6edb9c7-daca-48e9-9642-8f6202eaf2ec)
--
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]
