Copilot commented on code in PR #35964:
URL: https://github.com/apache/superset/pull/35964#discussion_r2501372494
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -470,12 +505,28 @@ const FiltersConfigForm = (
}
})
.catch((error: Response) => {
- getClientErrorObject(error).then(clientErrorObject => {
- setError(clientErrorObject);
- });
+ if (isMountedRef.current && requestId >= latestRequestIdRef.current)
{
+ getClientErrorObject(error).then(clientErrorObject => {
+ if (
+ isMountedRef.current &&
+ requestId >= latestRequestIdRef.current
+ ) {
+ setError(clientErrorObject);
+ }
+ });
+ }
});
},
- [filterId, forceUpdate, form, formFilter, hasDataset, dependenciesText],
+ [
+ filterId,
+ forceUpdate,
+ form,
+ formFilter,
+ hasDataset,
+ dependenciesText,
+ isMountedRef,
+ latestRequestIdRef,
Review Comment:
The refs `isMountedRef` and `latestRequestIdRef` should not be included in
the `useCallback` dependency array. Refs are mutable objects whose identity
doesn't change between renders, so including them in dependencies is
unnecessary and could confuse readers about whether changes to ref values
trigger callback recreation (they don't). Only include dependencies that, when
changed, should cause the callback to be recreated.
```suggestion
// isMountedRef and latestRequestIdRef intentionally omitted; their
identity does not change
```
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.test.tsx:
##########
@@ -0,0 +1,914 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { act, render, waitFor } from 'spec/helpers/testing-library';
+import { FormInstance } from 'antd/lib/form';
+import { Preset, isFeatureEnabled, FeatureFlag } from '@superset-ui/core';
+import { getChartDataRequest } from 'src/components/Chart/chartAction';
+import { waitForAsyncData } from 'src/middleware/asyncEvent';
+import chartQueries from 'spec/fixtures/mockChartQueries';
+import mockDatasource, { datasourceId, id } from
'spec/fixtures/mockDatasource';
+import {
+ SelectFilterPlugin,
+ RangeFilterPlugin,
+ TimeFilterPlugin,
+ TimeColumnFilterPlugin,
+ TimeGrainFilterPlugin,
+} from 'src/filters/components';
+import FiltersConfigForm, { FiltersConfigFormProps } from
'./FiltersConfigForm';
+
+// Mock external dependencies
+jest.mock('src/components/Chart/chartAction');
+jest.mock('src/middleware/asyncEvent');
+jest.mock('@superset-ui/core', () => ({
+ ...jest.requireActual('@superset-ui/core'),
+ isFeatureEnabled: jest.fn(),
+}));
+
+const mockGetChartDataRequest = getChartDataRequest as jest.Mock;
+const mockWaitForAsyncData = waitForAsyncData as jest.Mock;
+const mockIsFeatureEnabled = isFeatureEnabled as jest.Mock;
+
+// Type for chart data response structure
+type MockChartDataResponse = {
+ response: { status: number };
+ json: {
+ result: Array<{
+ status?: string;
+ data?: unknown[];
+ applied_filters?: unknown[];
+ channel_id?: string;
+ job_id?: string;
+ user_id?: string;
+ errors?: unknown[];
+ }>;
+ };
+};
+
+// Register filter plugins
+class FilterPreset extends Preset {
+ constructor() {
+ super({
+ name: 'Filter plugins',
+ plugins: [
+ new SelectFilterPlugin().configure({ key: 'filter_select' }),
+ new RangeFilterPlugin().configure({ key: 'filter_range' }),
+ new TimeFilterPlugin().configure({ key: 'filter_time' }),
+ new TimeColumnFilterPlugin().configure({ key: 'filter_timecolumn' }),
+ new TimeGrainFilterPlugin().configure({ key: 'filter_timegrain' }),
+ ],
+ });
+ }
+}
+
+// Test fixtures
+const defaultState = () => ({
+ datasources: { ...mockDatasource },
+ charts: chartQueries,
+ dashboardLayout: {
+ present: {},
+ past: [],
+ future: [],
+ },
+ dashboardInfo: {
+ id: 123,
+ },
+});
+
+function createMockChartResponse(
+ data = [{ name: 'Aaron', count: 453 }],
+): MockChartDataResponse {
+ return {
+ response: { status: 200 },
+ json: {
+ result: [
+ {
+ status: 'success',
+ data,
+ applied_filters: [{ column: 'name' }],
+ },
+ ],
+ },
+ };
+}
+
+function createMockAsyncChartResponse(): MockChartDataResponse {
+ return {
+ response: { status: 202 },
+ json: {
+ result: [
+ {
+ channel_id: 'test-channel-123',
+ job_id: 'test-job-456',
+ user_id: '1',
+ status: 'pending',
+ errors: [],
+ },
+ ],
+ },
+ };
+}
+
+function createFormInstance(): FormInstance {
+ const formData: Record<string, unknown> = {};
+
+ const formInstance = {
+ getFieldValue: jest.fn((path: string | string[]) => {
+ const keys = Array.isArray(path) ? path : [path];
+ let value: unknown = formData;
+ for (const key of keys) {
+ value = (value as Record<string, unknown>)?.[key];
+ }
+ // Return a deep copy to ensure new object references
+ return value ? JSON.parse(JSON.stringify(value)) : value;
+ }),
+ getFieldsValue: jest.fn(() => JSON.parse(JSON.stringify(formData))),
+ getFieldError: jest.fn(() => []),
+ getFieldsError: jest.fn(() => []),
+ getFieldWarning: jest.fn(() => []),
+ isFieldsTouched: jest.fn(() => false),
+ isFieldTouched: jest.fn(() => false),
+ isFieldValidating: jest.fn(() => false),
+ isFieldsValidating: jest.fn(() => false),
+ resetFields: jest.fn(),
+ setFields: jest.fn(
+ (fields: Array<{ name: string | string[]; value: unknown }>) => {
+ fields.forEach(field => {
+ const keys = Array.isArray(field.name) ? field.name : [field.name];
+ let target: Record<string, unknown> = formData;
+ // eslint-disable-next-line no-plusplus
+ for (let i = 0; i < keys.length - 1; i++) {
+ if (!target[keys[i]]) target[keys[i]] = {};
+ target = target[keys[i]] as Record<string, unknown>;
+ }
+ target[keys[keys.length - 1]] = field.value;
+ });
+ },
+ ),
+ setFieldValue: jest.fn((path: string | string[], value: unknown) => {
+ const keys = Array.isArray(path) ? path : [path];
+ let target: Record<string, unknown> = formData;
+ // eslint-disable-next-line no-plusplus
+ for (let i = 0; i < keys.length - 1; i++) {
+ if (!target[keys[i]]) target[keys[i]] = {};
+ target = target[keys[i]] as Record<string, unknown>;
+ }
+ target[keys[keys.length - 1]] = value;
+ }),
+ setFieldsValue: jest.fn((values: Record<string, unknown>) => {
+ Object.assign(formData, values);
+ }),
+ validateFields: jest.fn().mockResolvedValue({}),
+ submit: jest.fn(),
+ scrollToField: jest.fn(),
+ getInternalHooks: jest.fn(() => ({
+ dispatch: jest.fn(),
+ initEntityValue: jest.fn(),
+ registerField: jest.fn(),
+ useSubscribe: jest.fn(),
+ setInitialValues: jest.fn(),
+ setCallbacks: jest.fn(),
+ setValidateMessages: jest.fn(),
+ getFields: jest.fn(() => []),
+ setPreserve: jest.fn(),
+ getInitialValue: jest.fn(),
+ })),
+ __INTERNAL__: {
+ name: 'test-form',
+ },
+ focusField: jest.fn(),
+ getFieldInstance: jest.fn(),
+ } as FormInstance;
+
+ return formInstance;
+}
+
+function createDefaultProps(form: FormInstance): FiltersConfigFormProps {
+ return {
+ expanded: false,
+ filterId: 'NATIVE_FILTER-1',
+ removedFilters: {},
+ restoreFilter: jest.fn(),
+ onModifyFilter: jest.fn(),
+ form,
+ getAvailableFilters: jest.fn(() => []),
+ handleActiveFilterPanelChange: jest.fn(),
+ activeFilterPanelKeys: [],
+ isActive: true,
+ setErroredFilters: jest.fn(),
+ validateDependencies: jest.fn(),
+ getDependencySuggestion: jest.fn(() => ''),
+ };
+}
+
+let form: FormInstance;
+
+beforeAll(() => {
+ // Register filter plugins for the test suite
+ new FilterPreset().register();
+});
+
+beforeEach(() => {
+ jest.clearAllMocks();
+ form = createFormInstance();
+ // Default to synchronous queries (no GlobalAsyncQueries)
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag !== FeatureFlag.GlobalAsyncQueries,
+ );
+ // Default mock response
+ mockGetChartDataRequest.mockResolvedValue(createMockChartResponse());
+});
+
+test('should render without crashing with required props', () => {
+ const props = createDefaultProps(form);
+ const { container } = render(<FiltersConfigForm {...props} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ expect(container).toBeInTheDocument();
+});
+
+test('should handle synchronous query responses correctly', async () => {
+ const testData = [
+ { name: 'Alice', count: 100 },
+ { name: 'Bob', count: 200 },
+ ];
+ mockGetChartDataRequest.mockResolvedValue(createMockChartResponse(testData));
+
+ const props = createDefaultProps(form);
+ const { filterId } = props;
+
+ // Set up form with dataset and column to trigger refresh
+ act(() => {
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'name',
+ },
+ },
+ });
+ });
+
+ render(<FiltersConfigForm {...props} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Wait for the refresh to complete
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+
+ // Verify waitForAsyncData was NOT called for sync response
+ expect(mockWaitForAsyncData).not.toHaveBeenCalled();
+
+ // Verify form was updated with the response data
+ const formValues = form.getFieldValue('filters')?.[filterId];
+ expect(formValues.defaultValueQueriesData).toBeDefined();
+});
+
+test('should refresh filter data when dataset changes without default value
enabled', async () => {
+ const filterId = 'NATIVE_FILTER-1';
+
+ // Test with dataset A (use numeric ID, not datasourceId string)
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'name',
+ controlValues: {
+ enableEmptyFilter: false,
+ defaultToFirstItem: false,
+ },
+ },
+ },
+ });
+
+ const propsA = createDefaultProps(form);
+ const { unmount } = render(<FiltersConfigForm {...propsA} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Wait for initial refresh with dataset A
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+
+ // Verify correct dataset ID in the API call for dataset A
+ const firstCall = mockGetChartDataRequest.mock.calls[0];
+ expect(firstCall[0].formData.datasource).toBe(datasourceId);
+
+ const callsAfterDatasetA = mockGetChartDataRequest.mock.calls.length;
+ unmount();
+
+ // Now test with dataset B - creates new component instance
+ const newDatasetNumericId = id + 1; // 8
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: newDatasetNumericId }, // Pass numeric ID
+ column: 'name',
+ controlValues: {
+ enableEmptyFilter: false,
+ defaultToFirstItem: false,
+ },
+ },
+ },
+ });
+
+ const propsB = createDefaultProps(form);
+ render(<FiltersConfigForm {...propsB} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Verify refresh called again with correct dataset B
+ await waitFor(() => {
+ expect(mockGetChartDataRequest.mock.calls.length).toBeGreaterThan(
+ callsAfterDatasetA,
+ );
+ });
+
+ // Verify the new call has the updated dataset ID
+ const lastCallB =
+ mockGetChartDataRequest.mock.calls[
+ mockGetChartDataRequest.mock.calls.length - 1
+ ];
+ expect(lastCallB[0].formData.datasource).toBe(
+ `${newDatasetNumericId}__table`,
+ );
+});
+
+test('should refresh when dataset changes even with default value enabled',
async () => {
+ const filterId = 'NATIVE_FILTER-1';
+
+ // Test with dataset A WITH default value
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'name',
+ controlValues: {
+ enableEmptyFilter: true, // This enables "Filter has default value"
+ defaultToFirstItem: false,
+ },
+ defaultDataMask: {
+ filterState: {
+ value: ['Aaron'],
+ },
+ },
+ },
+ },
+ });
+
+ const propsA = createDefaultProps(form);
+ const { unmount } = render(<FiltersConfigForm {...propsA} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Wait for initial refresh
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+
+ const callsAfterDatasetA = mockGetChartDataRequest.mock.calls.length;
+ unmount();
+
+ // CRITICAL TEST: Change to dataset B - verifies the bug fix
+ // Before fix: hasDefaultValue would block refresh
+ // After fix: refresh happens regardless of hasDefaultValue
+ const newDatasetNumericId = id + 1; // 8
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: newDatasetNumericId }, // numeric ID: 8
+ column: 'name',
+ controlValues: {
+ enableEmptyFilter: true,
+ defaultToFirstItem: false,
+ },
+ defaultDataMask: {
+ filterState: {
+ value: ['Aaron'],
+ },
+ },
+ },
+ },
+ });
+
+ const propsB = createDefaultProps(form);
+ render(<FiltersConfigForm {...propsB} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Verify refresh called again with dataset B
+ await waitFor(() => {
+ expect(mockGetChartDataRequest.mock.calls.length).toBeGreaterThan(
+ callsAfterDatasetA,
+ );
+ });
+});
+
+test('should refresh when column changes and verify isDataDirty state
transition', async () => {
+ const filterId = 'NATIVE_FILTER-1';
+
+ // Initial setup with column 'name'
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'name',
+ },
+ },
+ });
+
+ const propsA = createDefaultProps(form);
+ const { unmount } = render(<FiltersConfigForm {...propsA} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Wait for initial refresh
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+
+ // Verify column 'name' in first call
+ const firstCall = mockGetChartDataRequest.mock.calls[0];
+ expect(firstCall[0].formData.groupby).toEqual(['name']);
+
+ const initialCallCount = mockGetChartDataRequest.mock.calls.length;
+ unmount();
+
+ // Change to column 'gender' - should set isDataDirty
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'gender',
+ isDataDirty: true, // useBackendFormUpdate hook would set this
+ },
+ },
+ });
+
+ const propsB = createDefaultProps(form);
+ render(<FiltersConfigForm {...propsB} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Verify refresh called again after column change
+ await waitFor(() => {
+ expect(mockGetChartDataRequest.mock.calls.length).toBeGreaterThan(
+ initialCallCount,
+ );
+ });
+
+ // Verify the new call has updated column
+ const lastCall =
+ mockGetChartDataRequest.mock.calls[
+ mockGetChartDataRequest.mock.calls.length - 1
+ ];
+ expect(lastCall[0].formData.groupby).toEqual(['gender']);
+
+ // Verify isDataDirty was reset to false after refresh completed
+ await waitFor(() => {
+ const finalFormValues = form.getFieldValue('filters')?.[filterId];
+ // After refreshHandler completes, isDataDirty should be false
+ expect(finalFormValues.isDataDirty).toBe(false);
+ });
+});
+
+test('should handle async query responses with polling', async () => {
+ // Enable GlobalAsyncQueries feature flag
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GlobalAsyncQueries,
+ );
+
+ // Mock async response (202)
+ mockGetChartDataRequest.mockResolvedValue(createMockAsyncChartResponse());
+
+ // Mock successful polling result
+ const asyncData = [
+ {
+ status: 'success',
+ data: [{ name: 'Async Result', count: 999 }],
+ applied_filters: [],
+ },
+ ];
+ mockWaitForAsyncData.mockResolvedValue(asyncData);
+
+ const props = createDefaultProps(form);
+ const { filterId } = props;
+
+ act(() => {
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'name',
+ },
+ },
+ });
+ });
+
+ render(<FiltersConfigForm {...props} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Verify async flow
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+
+ await waitFor(() => {
+ expect(mockWaitForAsyncData).toHaveBeenCalled();
+ });
+
+ // Verify final data is set
+ await waitFor(() => {
+ const formValues = form.getFieldValue('filters')?.[filterId];
+ expect(formValues.defaultValueQueriesData).toEqual(asyncData);
+ });
+});
+
+test('should display error when API request fails', async () => {
+ // Mock API error with a simple error object
+ const mockError = {
+ error: 'Internal Server Error',
+ message: 'An error occurred',
+ statusText: 'Internal Server Error',
+ };
+ mockGetChartDataRequest.mockRejectedValue(mockError);
+
+ const filterId = 'NATIVE_FILTER-1';
+
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'name',
+ },
+ },
+ });
+
+ const props = createDefaultProps(form);
+ const { container } = render(<FiltersConfigForm {...props} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ });
+
+ // Wait a bit for error handling to complete
+ await new Promise(resolve => setTimeout(resolve, 100));
+
+ // Component should still be rendered (no crash)
+ expect(container).toBeInTheDocument();
+
+ // defaultValueQueriesData should remain null on error
+ const formValues = form.getFieldValue('filters')?.[filterId];
+ expect(formValues.defaultValueQueriesData).toBeNull();
+
+ // Verify API was called with correct parameters
+ expect(mockGetChartDataRequest).toHaveBeenCalledWith(
+ expect.objectContaining({
+ formData: expect.objectContaining({
+ datasource: datasourceId,
+ groupby: ['name'],
+ }),
+ }),
+ );
+
+ // Note: setErroredFilters is only called during form validation, NOT during
refresh errors
+ // The refresh error handler calls setError/setErrorWrapper instead
+ // Verify error UI is displayed
+ await waitFor(() => {
+ const formValues = form.getFieldValue('filters')?.[filterId];
+ // The component sets error through setError() which isn't reflected in
form state
+ // But we've verified the component didn't crash and
defaultValueQueriesData is null
+ expect(formValues.defaultValueQueriesData).toBeNull();
+ });
+
+ // TODO: Once component properly renders error state, add this assertion:
+ // const errorAlert = await screen.findByRole('alert');
+ // expect(errorAlert).toHaveTextContent('An error occurred');
+});
+
+test('should cleanup when unmounted during async operation', async () => {
+ mockIsFeatureEnabled.mockImplementation(
+ (flag: FeatureFlag) => flag === FeatureFlag.GlobalAsyncQueries,
+ );
+
+ mockGetChartDataRequest.mockResolvedValue(createMockAsyncChartResponse());
+
+ // Mock a slow async operation
+ let asyncResolve!: (value: MockChartDataResponse) => void;
+ const asyncPromise = new Promise<MockChartDataResponse>(resolve => {
+ asyncResolve = resolve;
+ });
+ mockWaitForAsyncData.mockReturnValue(asyncPromise);
+
+ const props = createDefaultProps(form);
+ const { filterId } = props;
+
+ // Spy on console errors to catch React warnings about state updates after
unmount
+ const consoleErrorSpy = jest
+ .spyOn(console, 'error')
+ .mockImplementation(() => {});
+
+ act(() => {
+ form.setFieldsValue({
+ filters: {
+ [filterId]: {
+ filterType: 'filter_select',
+ dataset: { value: id }, // numeric ID: 7
+ column: 'name',
+ },
+ },
+ });
+ });
+
+ const { unmount } = render(<FiltersConfigForm {...props} />, {
+ initialState: defaultState(),
+ useRedux: true,
+ });
+
+ // Wait for async operation to start
+ await waitFor(() => {
+ expect(mockGetChartDataRequest).toHaveBeenCalled();
+ expect(mockWaitForAsyncData).toHaveBeenCalled();
+ });
+
+ // Unmount before async completes
+ unmount();
+
+ // Now resolve the async operation after unmount
+ asyncResolve(createMockChartResponse([{ name: 'Delayed', count: 1 }]));
Review Comment:
The type for `asyncResolve` and the promise don't match the expected return
type of `waitForAsyncData`. Based on line 526 where
`mockWaitForAsyncData.mockResolvedValue(asyncData)` expects an array of chart
results, the promise should be `Promise<unknown[]>` and `asyncResolve` should
accept `unknown[]`. Line 679 also incorrectly resolves with
`createMockChartResponse()` which returns a full response object instead of
just the result array.
```suggestion
asyncResolve([{ name: 'Delayed', count: 1 }]);
```
--
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]
