Copilot commented on code in PR #33960:
URL: https://github.com/apache/superset/pull/33960#discussion_r2173604302
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -457,6 +457,38 @@ function FiltersConfigModal({
}
}, [form, erroredFilters]);
+ /**
+ * Helper function to find dependency validation errors in form fields.
+ * This function improves code maintainability by centralizing the logic
+ * for detecting cyclic dependency errors and extracting the problematic
filter ID.
+ *
+ * @param fields - Array of form field error objects from Ant Design Form
+ * @returns Object containing error status and filter ID, or null if no
dependency errors
+ */
+ const findDependencyError = (fields: any[]) => {
Review Comment:
[nitpick] Extract `findDependencyError` into a shared utility (e.g.,
`utils.ts`) so that it can be unit tested in isolation and reused across
components.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -457,6 +457,38 @@ function FiltersConfigModal({
}
}, [form, erroredFilters]);
+ /**
+ * Helper function to find dependency validation errors in form fields.
+ * This function improves code maintainability by centralizing the logic
+ * for detecting cyclic dependency errors and extracting the problematic
filter ID.
+ *
+ * @param fields - Array of form field error objects from Ant Design Form
+ * @returns Object containing error status and filter ID, or null if no
dependency errors
+ */
+ const findDependencyError = (fields: any[]) => {
Review Comment:
Consider strongly typing the `fields` parameter (e.g., use Ant Design's
FieldError type) and destructuring `field.name` into meaningful variables
instead of relying on numeric indexes to improve readability and reduce
fragility.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx:
##########
@@ -584,3 +584,199 @@ test('modifies the name of a filter', async () => {
),
);
});
+
+test('prevents saving cyclic dependencies created indirectly', async () => {
+ // Test case: Reproduce exact scenario from GitHub issue #33908
+ // This test verifies that users cannot create A→B dependency, save it,
+ // then later add B→A dependency and save again (which would create a cycle)
+
+ const nativeFilterState = [
+ buildNativeFilter('NATIVE_FILTER-1', 'state', []),
+ buildNativeFilter('NATIVE_FILTER-2', 'country', []),
+ ];
+ const state = {
+ ...defaultState(),
+ dashboardInfo: {
+ metadata: { native_filter_configuration: nativeFilterState },
+ },
+ dashboardLayout,
+ };
+ const onSave = jest.fn();
+ defaultRender(state, {
+ ...props,
+ createNewOnOpen: false,
+ onSave,
+ });
+
+ // STEP 1: Set up Filter 1 → depends on Filter 2 (should succeed)
+ const filterTabs = screen.getAllByRole('tab');
+ userEvent.click(filterTabs[0]); // Select first filter
+
+ // Enable the dependencies option for Filter 1
+ const dependenciesCheckbox = screen.getByRole('checkbox', { name:
DEPENDENCIES_REGEX });
+ userEvent.click(dependenciesCheckbox);
+
+ // Set Filter 1 to depend on Filter 2
+ const dependencySelect = screen.getByRole('combobox', { name: /values
dependent on/i });
+ userEvent.click(dependencySelect);
+ const filter2Option = screen.getByText('country');
+ userEvent.click(filter2Option);
+
+ // Save this configuration (should work as there's no cycle yet)
+ userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
+ await waitFor(() => expect(onSave).toHaveBeenCalledTimes(1));
+
+ // Verify the save was successful with correct dependency structure
+ expect(onSave).toHaveBeenCalledWith(
+ expect.objectContaining({
+ filterConfig: expect.objectContaining({
+ 'NATIVE_FILTER-1': expect.objectContaining({
+ dependsOn: ['NATIVE_FILTER-2'], // Filter 1 now depends on Filter 2
+ }),
+ 'NATIVE_FILTER-2': expect.objectContaining({
+ dependsOn: [], // Filter 2 has no dependencies
+ }),
+ }),
+ }),
+ );
+
+ // Reset mock for second save attempt
+ onSave.mockClear();
+
+ // STEP 2: Simulate reopening the modal and trying to create reverse
dependency
+ // This reproduces the bug scenario: user reopens modal to edit Filter 2
+ defaultRender(state, {
+ ...props,
+ createNewOnOpen: false,
+ onSave,
+ });
+
+ const filterTabs2 = screen.getAllByRole('tab');
+ userEvent.click(filterTabs2[1]); // Select second filter (Filter 2)
+
+ // Try to set Filter 2 to depend on Filter 1 (creating A→B→A cycle)
+ const dependenciesCheckbox2 = screen.getByRole('checkbox', { name:
DEPENDENCIES_REGEX });
+ userEvent.click(dependenciesCheckbox2);
+
+ const dependencySelect2 = screen.getByRole('combobox', { name: /values
dependent on/i });
+ userEvent.click(dependencySelect2);
+ const filter1Option = screen.getByText('state');
+ userEvent.click(filter1Option);
+
+ // STEP 3: Attempt to save the cyclic configuration (should be blocked)
+ userEvent.click(screen.getByRole('button', { name: SAVE_REGEX }));
+
+ // Verify that save was prevented due to cyclic dependency detection
+ await waitFor(() => {
+ expect(onSave).not.toHaveBeenCalled();
+ }, { timeout: 3000 });
+
+ // Verify that user receives appropriate error feedback
+ await waitFor(() => {
+ expect(screen.getByText(/cyclic dependency
detected/i)).toBeInTheDocument();
+ }, { timeout: 3000 });
+
+ // Verify that focus is moved to the problematic filter for user guidance
+ await waitFor(() => {
+ const activeTab = screen.getByRole('tab', { selected: true });
+ expect(activeTab).toHaveTextContent('country'); // Should focus on Filter 2
+ }, { timeout: 3000 });
Review Comment:
[nitpick] Consider reducing the `waitFor` timeout from 3000ms to a lower
value to speed up CI runs, as the UI should update quickly after validation.
```suggestion
}, { timeout: 1000 });
// Verify that user receives appropriate error feedback
await waitFor(() => {
expect(screen.getByText(/cyclic dependency
detected/i)).toBeInTheDocument();
}, { timeout: 1000 });
// Verify that focus is moved to the problematic filter for user guidance
await waitFor(() => {
const activeTab = screen.getByRole('tab', { selected: true });
expect(activeTab).toHaveTextContent('country'); // Should focus on
Filter 2
}, { timeout: 1000 });
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
