You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sadpandajoe (via GitHub)" <gi...@apache.org> on 2025/11/03 22:07:41 UTC
[PR] fix(native-filters): frontend filters not refreshing after switching datasets [superset]
sadpandajoe opened a new pull request, #35964:
URL: https://github.com/apache/superset/pull/35964
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue: Fixes #35674
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): native filter refresh when dataset changes [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #35964:
URL: https://github.com/apache/superset/pull/35964#issuecomment-3513309002
Closing in favor of https://github.com/apache/superset/pull/35488
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native-filters): frontend filters not refreshing after switching datasets [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35964:
URL: https://github.com/apache/superset/pull/35964#issuecomment-3482782939
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(native-filters): frontend filters not refreshing after switching datasets [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #35964:
URL: https://github.com/apache/superset/pull/35964#issuecomment-3482782701
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](5cef440b-3217-4523-ba4b-e32fe9663278)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): native filter refresh when dataset changes [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): native filter refresh when dataset changes [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe closed pull request #35964: fix(dashboard): native filter refresh when dataset changes
URL: https://github.com/apache/superset/pull/35964
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): native filter refresh when dataset changes [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #35964:
URL: https://github.com/apache/superset/pull/35964#discussion_r2501071552
##########
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 `isMountedRef` and `latestRequestIdRef` should not be included in the dependency array of `useCallback`. Refs are stable and don't trigger re-renders, so including them serves no purpose and may confuse readers about the actual dependencies. Remove them from the dependency array.
```suggestion
// isMountedRef and latestRequestIdRef are refs and should not be included
```
##########
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
Review Comment:
The license header is incomplete. Line 14 should read "AS IS" BASIS instead of just "IS" BASIS.
```suggestion
* AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
```
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): native filter refresh when dataset changes [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35964:
URL: https://github.com/apache/superset/pull/35964#issuecomment-3505211195
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19182521503) for [3cdc0b7](https://github.com/apache/superset/commit/3cdc0b7)
• **Environment:** http://18.237.248.193:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dashboard): native filter refresh when dataset changes [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35964:
URL: https://github.com/apache/superset/pull/35964#issuecomment-3505177764
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19182521503) for [3cdc0b7](https://github.com/apache/superset/commit/3cdc0b7)
--
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-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org