You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rabumaabraham (via GitHub)" <gi...@apache.org> on 2026/09/29 15:49:41 UTC
[PR] fix(native-filters): preserve selected filter state [superset]
rabumaabraham opened a new pull request, #44803:
URL: https://github.com/apache/superset/pull/44803
<!---
fix(native-filters): preserve selected filter state
-->
### SUMMARY
The FilterBar previously removed selected filter state when the filter was missing from dataMaskApplied. This could clear the displayed selection even though the filter still existed in the dashboard configuration.
The cleanup now removes state only when the filter no longer exists in the configured filters.
A regression test was added to verify that a configured filter keeps its selected value after its applied data mask is removed.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### Before
<img width="1917" height="1032" alt="32b676c6-969d-4316-8f08-051546781738" src="https://github.com/user-attachments/assets/c90da9b8-fab5-4d3e-993f-a82caeb210ba" />
#### After
<img width="1917" height="1032" alt="image" src="https://github.com/user-attachments/assets/c3703a1e-7bce-469c-b498-6effa9196a5c" />
### TESTING INSTRUCTIONS
1. Open a dashboard containing a Native FilterBar with a filter such as Current Month.
2. Verify that the filter displays its selected value.
3. Change or clear the filter and verify that the dashboard filtering continues to work correctly.
4. Return to the filter and verify that the selected value is displayed correctly instead of No filter.
5. Refresh the dashboard and verify that the filter state remains correct.
#### Automated test:
npm test -- -t "FilterBar" src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx
Result:
Test Suites: 1 passed, 1 total
Tests: 7 skipped, 22 passed, 29 total
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] 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(native-filters): preserve selected filter state [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-5969348343
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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] test(native-filters): cover time filter apply flow from #44680 [superset]
Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-6304054976
@sadpandajoe description updated and CI is green, ready for another look when you have time. Thanks!
--
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] test(native-filters): cover time filter apply flow from #44680 [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4335156861
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1529,3 +1552,177 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+const TIME_FILTER_ID = 'NATIVE_FILTER-time-range';
+
+function mockTimeRangeRequests() {
+ setupTimeRangeMocks();
+ mockedFetchTimeRange.mockResolvedValue({
+ value: '2021-04-01T00:00:00 β€ ds < 2021-05-01T00:00:00',
+ });
+ fetchMock.removeRoute('time-filter-chart-data');
+ fetchMock.post(
+ 'glob:*/api/v1/chart/data',
+ {
+ result: [
+ {
+ data: [{ ds: '2021-04-14T00:00:00' }],
+ colnames: ['ds'],
+ coltypes: [2],
+ applied_filters: [],
+ },
+ ],
+ },
+ { name: 'time-filter-chart-data' },
+ );
+}
+
+function renderTimeFilterBar(
+ dataMask: Record<string, unknown>,
+ dashboardInfoOverrides: Record<string, unknown> = {},
+) {
+ const filter = createFilter({
+ id: TIME_FILTER_ID,
+ name: 'Time range',
+ filterType: 'filter_time',
+ targets: [{ datasetId: 7, column: { name: 'ds' } }],
+ chartsInScope: [18],
+ });
+ const state = {
+ ...createStateWithFilter(filter, createDataMask(TIME_FILTER_ID), {
+ filterBarOrientation: FilterBarOrientation.Horizontal,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ ...dashboardInfoOverrides,
+ }),
+ dataMask,
+ };
+ const store: Store = createStore(state, reducerIndex);
+
+ render(<FilterBar orientation={FilterBarOrientation.Horizontal} />, {
+ store,
+ useDnd: true,
+ useRouter: true,
+ });
+
+ return store;
+}
+
+const getAppliedTimeMask = (store: Store) =>
+ (store.getState() as RootState).dataMask[TIME_FILTER_ID];
+
+async function selectCurrentMonth(currentLabel: string) {
+ await userEvent.click(screen.getByText(currentLabel));
+ await userEvent.click(screen.getByLabelText('Range type'));
+ await userEvent.click(screen.getByText('Current'));
+ await userEvent.click(screen.getByRole('radio', { name: 'Current month' }));
+ await userEvent.click(
+ screen.getByTestId('date-filter-control__apply-button'),
+ );
+}
+
+async function applyAndClearCurrentMonth(store: Store) {
+ const applyButton = screen.getByTestId(getTestId('apply-button'));
+
+ await selectCurrentMonth('No filter');
+
+ // The selected range differs from the applied state, so Apply enables.
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+ expect(applyButton).toBeEnabled();
+
+ await userEvent.click(applyButton);
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(getAppliedTimeMask(store)).toEqual(
+ expect.objectContaining({
+ filterState: expect.objectContaining({ value: 'Current month' }),
+ extraFormData: { time_range: 'Current month' },
+ }),
+ );
+ // The applied range stays visible in the filter bar.
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+ expect(screen.queryByText('No filter')).not.toBeInTheDocument();
+ expect(applyButton).toBeDisabled();
+
+ // Clear All stays available and clears the applied range.
+ const clearAllButton = screen.getByRole('button', { name: 'Clear all' });
+ expect(clearAllButton).toBeEnabled();
+ await userEvent.click(clearAllButton);
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(applyButton).toBeEnabled();
+ await userEvent.click(applyButton);
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(getAppliedTimeMask(store)?.extraFormData).toEqual({});
+ expect(screen.getByText('No filter')).toBeInTheDocument();
+}
+
+test('FilterBar applies a time range selected from an empty applied entry and keeps its label', async () => {
+ mockTimeRangeRequests();
+ const store = renderTimeFilterBar({
+ [TIME_FILTER_ID]: {
+ id: TIME_FILTER_ID,
+ filterState: {},
+ extraFormData: {},
+ ownState: {},
+ },
+ });
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('No filter')).toBeInTheDocument();
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+
+ await applyAndClearCurrentMonth(store);
+});
+
+test('FilterBar applies a time range selected for a filter without an applied entry', async () => {
+ mockTimeRangeRequests();
+ const store = renderTimeFilterBar({});
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('No filter')).toBeInTheDocument();
+
+ await applyAndClearCurrentMonth(store);
+});
+
+test('FilterBar drops the selected time range when cross-filtering is disabled', async () => {
+ mockTimeRangeRequests();
+ const store = renderTimeFilterBar(
+ {
+ [TIME_FILTER_ID]: createDataMask(TIME_FILTER_ID, 'Last week', {
+ time_range: 'Last week',
+ }),
+ },
+ { crossFiltersEnabled: true },
+ );
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('Last week')).toBeInTheDocument();
+
+ await act(async () => {
+ store.dispatch(dataMaskActions.clearDataMaskState());
Review Comment:
The test dispatches `clearDataMaskState()` itself before flipping `crossFiltersEnabled`, so it never exercises the code that actually clears masks when a user disables cross-filtering (`updateCrossFiltersSetting` in `FilterBarSettings`). If that dispatch were dropped there, "Last week" would stay selected in the real UI and this test would still pass. Could this go through the settings toggle (or drop the manual dispatch) so the production path is what's covered?
--
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] test(native-filters): cover time filter apply flow from #44680 [superset]
Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4337729416
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1529,3 +1552,177 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+const TIME_FILTER_ID = 'NATIVE_FILTER-time-range';
+
+function mockTimeRangeRequests() {
+ setupTimeRangeMocks();
+ mockedFetchTimeRange.mockResolvedValue({
+ value: '2021-04-01T00:00:00 β€ ds < 2021-05-01T00:00:00',
+ });
+ fetchMock.removeRoute('time-filter-chart-data');
+ fetchMock.post(
+ 'glob:*/api/v1/chart/data',
+ {
+ result: [
+ {
+ data: [{ ds: '2021-04-14T00:00:00' }],
+ colnames: ['ds'],
+ coltypes: [2],
+ applied_filters: [],
+ },
+ ],
+ },
+ { name: 'time-filter-chart-data' },
+ );
+}
+
+function renderTimeFilterBar(
+ dataMask: Record<string, unknown>,
+ dashboardInfoOverrides: Record<string, unknown> = {},
+) {
+ const filter = createFilter({
+ id: TIME_FILTER_ID,
+ name: 'Time range',
+ filterType: 'filter_time',
+ targets: [{ datasetId: 7, column: { name: 'ds' } }],
+ chartsInScope: [18],
+ });
+ const state = {
+ ...createStateWithFilter(filter, createDataMask(TIME_FILTER_ID), {
+ filterBarOrientation: FilterBarOrientation.Horizontal,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ ...dashboardInfoOverrides,
+ }),
+ dataMask,
+ };
+ const store: Store = createStore(state, reducerIndex);
+
+ render(<FilterBar orientation={FilterBarOrientation.Horizontal} />, {
+ store,
+ useDnd: true,
+ useRouter: true,
+ });
+
+ return store;
+}
+
+const getAppliedTimeMask = (store: Store) =>
+ (store.getState() as RootState).dataMask[TIME_FILTER_ID];
+
+async function selectCurrentMonth(currentLabel: string) {
+ await userEvent.click(screen.getByText(currentLabel));
+ await userEvent.click(screen.getByLabelText('Range type'));
+ await userEvent.click(screen.getByText('Current'));
+ await userEvent.click(screen.getByRole('radio', { name: 'Current month' }));
+ await userEvent.click(
+ screen.getByTestId('date-filter-control__apply-button'),
+ );
+}
+
+async function applyAndClearCurrentMonth(store: Store) {
+ const applyButton = screen.getByTestId(getTestId('apply-button'));
+
+ await selectCurrentMonth('No filter');
+
+ // The selected range differs from the applied state, so Apply enables.
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+ expect(applyButton).toBeEnabled();
+
+ await userEvent.click(applyButton);
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(getAppliedTimeMask(store)).toEqual(
+ expect.objectContaining({
+ filterState: expect.objectContaining({ value: 'Current month' }),
+ extraFormData: { time_range: 'Current month' },
+ }),
+ );
+ // The applied range stays visible in the filter bar.
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+ expect(screen.queryByText('No filter')).not.toBeInTheDocument();
+ expect(applyButton).toBeDisabled();
+
+ // Clear All stays available and clears the applied range.
+ const clearAllButton = screen.getByRole('button', { name: 'Clear all' });
+ expect(clearAllButton).toBeEnabled();
+ await userEvent.click(clearAllButton);
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(applyButton).toBeEnabled();
+ await userEvent.click(applyButton);
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(getAppliedTimeMask(store)?.extraFormData).toEqual({});
+ expect(screen.getByText('No filter')).toBeInTheDocument();
+}
+
+test('FilterBar applies a time range selected from an empty applied entry and keeps its label', async () => {
+ mockTimeRangeRequests();
+ const store = renderTimeFilterBar({
+ [TIME_FILTER_ID]: {
+ id: TIME_FILTER_ID,
+ filterState: {},
+ extraFormData: {},
+ ownState: {},
+ },
+ });
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('No filter')).toBeInTheDocument();
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+
+ await applyAndClearCurrentMonth(store);
+});
+
+test('FilterBar applies a time range selected for a filter without an applied entry', async () => {
+ mockTimeRangeRequests();
+ const store = renderTimeFilterBar({});
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('No filter')).toBeInTheDocument();
+
+ await applyAndClearCurrentMonth(store);
+});
+
+test('FilterBar drops the selected time range when cross-filtering is disabled', async () => {
+ mockTimeRangeRequests();
+ const store = renderTimeFilterBar(
+ {
+ [TIME_FILTER_ID]: createDataMask(TIME_FILTER_ID, 'Last week', {
+ time_range: 'Last week',
+ }),
+ },
+ { crossFiltersEnabled: true },
+ );
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('Last week')).toBeInTheDocument();
+
+ await act(async () => {
+ store.dispatch(dataMaskActions.clearDataMaskState());
Review Comment:
@sadpandajoe, Thanks, updated. The test now turns cross-filtering off by clicking the toggle in the settings menu, so it goes through updateCrossFiltersSetting. I also tried removing the clearDataMaskState() call from FilterBarSettings: the test fails, and passes again with it back.
--
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): preserve selected filter state [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4135546202
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -391,11 +391,11 @@ const FilterBar: FC<FiltersBarProps> = ({
}
});
- // Remove stale entries that no longer exist in dataMaskApplied
+ // Remove stale entries that no longer exist in the configured filters
Object.keys(updated).forEach(filterId => {
if (
!isChartCustomization(filterId) &&
- !(filterId in dataMaskApplied)
+ !(filterId in filters)
) {
Review Comment:
**Suggestion:** This effect now reads `filters`, but `filters` is missing from its dependencies, so configuration-only changes leave stale selected entries until another state change occurs.
**Assessment:** π `Major` Β· π `Occurrence: Sometimes` Β· π·οΈ `Stale reference`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=969d2cb89d2b40009d7afdc4a79d3463&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=969d2cb89d2b40009d7afdc4a79d3463&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent π€ </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx
**Line:** 399:399
**Comment:**
*Stale Reference: This effect now reads `filters`, but `filters` is missing from its dependencies, so configuration-only changes leave stale selected entries until another state change occurs.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44803&comment_hash=54f79c0245dbd1c7f32bc3fb9d62c7e15de35fb0a43a0bfe1d63edcf4b65c28e&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44803&comment_hash=54f79c0245dbd1c7f32bc3fb9d62c7e15de35fb0a43a0bfe1d63edcf4b65c28e&reaction=dislike'>π</a>
--
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): preserve selected filter state [superset]
Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4161396085
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1413,73 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar keeps a configured filter selected when its applied data mask is removed', async () => {
+ fetchMock.post(
+ 'glob:*/api/v1/chart/data',
+ {
+ result: [
+ {
+ data: [{ region: 'East' }, { region: 'West' }],
+ colnames: ['region'],
+ coltypes: [1],
+ applied_filters: [],
+ },
+ ],
+ },
+ { name: 'configured-filter-selected-chart-data' },
+ );
+
+ const filterId = 'NATIVE_FILTER-keep-selected';
+ const filter = createFilter({
+ id: filterId,
+ name: 'Region',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 7, column: { name: 'region' } }],
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, ['East'], {
+ filters: [{ col: 'region', op: 'IN', val: ['East'] }],
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Vertical,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(
+ <FilterBar
+ orientation={FilterBarOrientation.Vertical}
+ verticalConfig={{
+ width: 280,
+ height: 400,
+ offset: 0,
+ ...createOpenedBarProps(),
+ }}
+ />,
+ { store, useDnd: true, useRouter: true },
+ );
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('Region')).toBeInTheDocument();
+ expect(screen.getByTitle('East')).toBeInTheDocument();
+
+ await act(async () => {
Review Comment:
Updated the test to cover the actual time-filter flow you mentioned.
It now selects **Current month** from the time filter, checks that Apply is enabled, verifies the applied Redux state and displayed label, and also covers Clear All.
I also checked it against the base commit. It fails on base and passes with the fix. The full FilterBar test suite passes 31/31.
This is included in `7b37b916d2`.
--
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): preserve selected filter state [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-5893803112
<!-- Bito Reply -->
The flagged issue is correct. The `useEffect` hook in `FilterBar/index.tsx` relies on the `filters` object, but it is missing from the dependency array, which can lead to stale state when configuration changes occur. To resolve this, add `filters` to the dependency array of the `useEffect` hook.
**superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx**
```
useEffect(() => {
// ... existing logic
}, [filters, ...otherDependencies]);
```
--
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): preserve selected filter state [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4135869902
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -391,11 +391,11 @@ const FilterBar: FC<FiltersBarProps> = ({
}
});
- // Remove stale entries that no longer exist in dataMaskApplied
+ // Remove stale entries that no longer exist in the configured filters
Object.keys(updated).forEach(filterId => {
if (
!isChartCustomization(filterId) &&
- !(filterId in dataMaskApplied)
+ !(filterId in filters)
) {
Review Comment:
β
**CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved** as of `68dc55e`.
Added `filters` to the effect dependency array, ensuring configuration-only filter changes trigger synchronization and stale entries are removed.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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): preserve selected filter state [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-5942319935
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/44803?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.55%. Comparing base ([`e65de1f`](https://app.codecov.io/gh/apache/superset/commit/e65de1fc0f0d99b32e02c4a094c48c7a32fb60a2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`7b37b91`](https://app.codecov.io/gh/apache/superset/commit/7b37b916d2e51df0d56d529186c860b58d691be0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 78 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #44803 +/- ##
==========================================
+ Coverage 81.53% 81.55% +0.02%
==========================================
Files 2974 2974
Lines 180320 180318 -2
Branches 41740 41738 -2
==========================================
+ Hits 147019 147067 +48
+ Misses 30579 30529 -50
Partials 2722 2722
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/44803/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/44803/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.08% <100.00%> (+0.05%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/44803?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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] test(native-filters): cover time filter apply flow from [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-6279214670
## CodeAnt PR Risk: Low Risk
- The PR appears safe to merge; the visible changes add focused regression tests without changing production behavior.
- The tests cover applying and clearing time ranges with empty or missing applied state, plus clearing a range when cross-filtering is disabled.
**Assessed commit:** `3e42b81b7d1d`
--
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): preserve selected filter state [superset]
Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4172619303
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1432,147 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar preserves a selected time range when its applied data mask is removed', async () => {
+ setupTimeRangeMocks();
+
+ mockedFetchTimeRange.mockResolvedValue({
+ value: '2021-04-07T00:00:00 β€ ds < 2021-04-14T00:00:00',
+ });
+
+ fetchMock.post('glob:*/api/v1/chart/data', {
+ result: [
+ {
+ data: [{ ds: '2021-04-14T00:00:00' }],
+ colnames: ['ds'],
+ coltypes: [2],
+ applied_filters: [],
+ },
+ ],
+ });
+
+ const filterId = 'NATIVE_FILTER-keep-time-range';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+
+ const filter = createFilter({
+ id: filterId,
+ name: 'Time range',
+ filterType: 'filter_time',
+ targets: [{ datasetId: 7, column: { name: 'ds' } }],
+ defaultDataMask: {
+ filterState: { value: 'Last week' },
+ extraFormData: { time_range: 'Last week' },
+ },
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, 'Last week', {
+ time_range: 'Last week',
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Horizontal,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(<FilterBar orientation={FilterBarOrientation.Horizontal} />, {
+ store,
+ useDnd: true,
+ useRouter: true,
+ });
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ await waitFor(() => {
+ expect(screen.queryByTestId('loading-indicator')).not.toBeInTheDocument();
+ });
+
+ expect(screen.getByText('Last week')).toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Last week'));
+
+ const rangeType = screen.getByLabelText('Range type');
+ await userEvent.click(rangeType);
+ await userEvent.click(screen.getByText('Current'));
+
+ await userEvent.click(screen.getByRole('radio', { name: 'Current month' }));
+
+ const timeFilterApply = screen.getByTestId(
+ 'date-filter-control__apply-button',
+ );
+ expect(timeFilterApply).not.toBeNull();
+ expect(timeFilterApply).not.toBeDisabled();
+
+ await userEvent.click(timeFilterApply!);
+
+ const filterBarApply = screen.getByTestId(getTestId('apply-button'));
+ expect(filterBarApply).toBeEnabled();
+
+ await userEvent.click(filterBarApply);
+
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(
+ filterId,
+ expect.objectContaining({
+ filterState: expect.objectContaining({
+ value: 'Current month',
+ }),
+ extraFormData: {
+ time_range: 'Current month',
+ },
+ }),
+ );
+
+ expect((store.getState() as RootState).dataMask[filterId]).toEqual(
Review Comment:
Addressed. I updated the Redux state assertion to use `(store.getState() as typeof state).dataMask[filterId]` to avoid the TypeScript type issue.
The regression test passes locally.
--
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] test(native-filters): cover time filter apply flow from [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-6301767675
> @sadpandajoe, Thanks for the thorough review, you were right about the test.
>
> I tried the exact flow you described (empty applied entry β Current month β Apply β Clear All) and it already passes on master without my change. The 6.1.0 bug came from the selectedCount !== appliedCount check in checkIsApplyDisabled, and #40470 removed that check on master.
>
> So I reverted my index.tsx change, which also takes care of the Undo, cross-filter and initial-load issues. I kept the tests for the real flow so the bug can't come back, and fixed the TypeScript errors and the shared mock. The PR is now tests only.
>
> Happy to close it instead if you don't think the extra tests are worth it.
@rabumaabraham it's up to you if you want to close this or not. If it adds other cases we don't have feel free to fix up the PR description and get CI passing and I can take another pass at it.
--
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] test(native-filters): cover time filter apply flow from [superset]
Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-6279222018
@sadpandajoe, Thanks for the thorough review, you were right about the test.
I tried the exact flow you described (empty applied entry β Current month β Apply β Clear All) and it already passes on master without my change. The 6.1.0 bug came from the selectedCount !== appliedCount check in checkIsApplyDisabled, and #40470 removed that check on master.
So I reverted my index.tsx change, which also takes care of the Undo, cross-filter and initial-load issues. I kept the tests for the real flow so the bug can't come back, and fixed the TypeScript errors and the shared mock. The PR is now tests only.
Happy to close it instead if you don't think the extra tests are worth it.
--
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): preserve selected filter state [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-5897239701
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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): preserve selected filter state [superset]
Posted by "rabumaabraham (via GitHub)" <gi...@apache.org>.
rabumaabraham commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4172621166
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -391,12 +391,9 @@ const FilterBar: FC<FiltersBarProps> = ({
}
});
- // Remove stale entries that no longer exist in dataMaskApplied
+ // Remove stale entries that no longer exist in the configured filters
Object.keys(updated).forEach(filterId => {
Review Comment:
Addressed. I added handling for the `true β false` cross-filtering transition so that `dataMaskSelected` is cleared when cross-filtering is disabled.
I also added a regression test covering this case. The test passes locally.
--
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] test(native-filters): cover time filter apply flow from #44680 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #44803:
URL: https://github.com/apache/superset/pull/44803
--
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): preserve selected filter state [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-5893696966
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): preserve selected filter state [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4159026430
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1413,73 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar keeps a configured filter selected when its applied data mask is removed', async () => {
+ fetchMock.post(
+ 'glob:*/api/v1/chart/data',
+ {
+ result: [
+ {
+ data: [{ region: 'East' }, { region: 'West' }],
+ colnames: ['region'],
+ coltypes: [1],
+ applied_filters: [],
+ },
+ ],
+ },
+ { name: 'configured-filter-selected-chart-data' },
+ );
+
+ const filterId = 'NATIVE_FILTER-keep-selected';
+ const filter = createFilter({
+ id: filterId,
+ name: 'Region',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 7, column: { name: 'region' } }],
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, ['East'], {
+ filters: [{ col: 'region', op: 'IN', val: ['East'] }],
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Vertical,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(
+ <FilterBar
+ orientation={FilterBarOrientation.Vertical}
+ verticalConfig={{
+ width: 280,
+ height: 400,
+ offset: 0,
+ ...createOpenedBarProps(),
+ }}
+ />,
+ { store, useDnd: true, useRouter: true },
+ );
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('Region')).toBeInTheDocument();
+ expect(screen.getByTitle('East')).toBeInTheDocument();
+
+ await act(async () => {
Review Comment:
This test can pass without fixing #44680βs horizontal time-filter flow: it removes an applied Select mask, whereas the reported failure loses the displayed range while the applied time mask remains present. Could we add a real-store test selecting Current month through the time popup, checking that dashboard Apply enables, then asserting the applied Redux range, retained label, and Clear All behavior, with fail-on-base/pass-on-head verification?
--
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): preserve selected filter state [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-5943266401
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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): preserve selected filter state [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44803:
URL: https://github.com/apache/superset/pull/44803#issuecomment-5940939071
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 7f5c9e3608e4874e8b556c5a9c84f1581986f9d3 |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6abecf1e3265d1000820157b |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-44803--superset-docs-preview.netlify.app](https://deploy-preview-44803--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">π±</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">π€</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLCoAgFAXQvbxxkhZWtIOW4edKgpXYI4po7zk7g_NSLseWmWaihmwxu1urQ7zb3XC8IEJMjCJOJDiGFycbRr0eOR3P4usejIULCn03aK-klFMnlR4tfT8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44803--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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): preserve selected filter state [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4161606719
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1432,147 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar preserves a selected time range when its applied data mask is removed', async () => {
+ setupTimeRangeMocks();
+
+ mockedFetchTimeRange.mockResolvedValue({
+ value: '2021-04-07T00:00:00 β€ ds < 2021-04-14T00:00:00',
+ });
+
+ fetchMock.post('glob:*/api/v1/chart/data', {
+ result: [
+ {
+ data: [{ ds: '2021-04-14T00:00:00' }],
+ colnames: ['ds'],
+ coltypes: [2],
+ applied_filters: [],
+ },
+ ],
+ });
+
+ const filterId = 'NATIVE_FILTER-keep-time-range';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+
+ const filter = createFilter({
+ id: filterId,
+ name: 'Time range',
+ filterType: 'filter_time',
+ targets: [{ datasetId: 7, column: { name: 'ds' } }],
+ defaultDataMask: {
+ filterState: { value: 'Last week' },
+ extraFormData: { time_range: 'Last week' },
+ },
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, 'Last week', {
+ time_range: 'Last week',
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Horizontal,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(<FilterBar orientation={FilterBarOrientation.Horizontal} />, {
+ store,
+ useDnd: true,
+ useRouter: true,
+ });
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ await waitFor(() => {
+ expect(screen.queryByTestId('loading-indicator')).not.toBeInTheDocument();
+ });
+
+ expect(screen.getByText('Last week')).toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Last week'));
+
+ const rangeType = screen.getByLabelText('Range type');
+ await userEvent.click(rangeType);
+ await userEvent.click(screen.getByText('Current'));
+
+ await userEvent.click(screen.getByRole('radio', { name: 'Current month' }));
+
+ const timeFilterApply = screen.getByTestId(
+ 'date-filter-control__apply-button',
+ );
+ expect(timeFilterApply).not.toBeNull();
+ expect(timeFilterApply).not.toBeDisabled();
+
+ await userEvent.click(timeFilterApply!);
+
+ const filterBarApply = screen.getByTestId(getTestId('apply-button'));
+ expect(filterBarApply).toBeEnabled();
+
+ await userEvent.click(filterBarApply);
+
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(
+ filterId,
+ expect.objectContaining({
+ filterState: expect.objectContaining({
+ value: 'Current month',
+ }),
+ extraFormData: {
+ time_range: 'Current month',
+ },
+ }),
+ );
+
+ expect((store.getState() as RootState).dataMask[filterId]).toEqual(
Review Comment:
The new `RootState` assertion fails TypeScript checking (TS2352): `createStore` accepts its reducers as `object`, so its inferred state exposes only `queryApi`, not the dashboard fields. Could we type the test store/state assertion compatibly so the frontend type-check passes?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1413,73 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar keeps a configured filter selected when its applied data mask is removed', async () => {
+ fetchMock.post(
+ 'glob:*/api/v1/chart/data',
+ {
+ result: [
+ {
+ data: [{ region: 'East' }, { region: 'West' }],
+ colnames: ['region'],
+ coltypes: [1],
+ applied_filters: [],
+ },
+ ],
+ },
+ { name: 'configured-filter-selected-chart-data' },
+ );
+
+ const filterId = 'NATIVE_FILTER-keep-selected';
+ const filter = createFilter({
+ id: filterId,
+ name: 'Region',
+ filterType: 'filter_select',
+ targets: [{ datasetId: 7, column: { name: 'region' } }],
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, ['East'], {
+ filters: [{ col: 'region', op: 'IN', val: ['East'] }],
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Vertical,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(
+ <FilterBar
+ orientation={FilterBarOrientation.Vertical}
+ verticalConfig={{
+ width: 280,
+ height: 400,
+ offset: 0,
+ ...createOpenedBarProps(),
+ }}
+ />,
+ { store, useDnd: true, useRouter: true },
+ );
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByText('Region')).toBeInTheDocument();
+ expect(screen.getByTitle('East')).toBeInTheDocument();
+
+ await act(async () => {
Review Comment:
The fail-on-base result can still come solely from the explicit `removeDataMask` step: the popup flow starts with an already-applied Last week range, whereas #44680 starts with an empty applied entry and loses the label while that entry remains present. Could we also cover that empty-entry β Current month β Apply β retained label and Clear All sequence without directly deleting the applied mask, and verify that sequence fails on base?
--
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): preserve selected filter state [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4174951472
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1232,7 +1254,7 @@ test('required filter with a default value auto-applies on load without touching
// Nothing is left pending: the default value is already applied, so the
// Apply button is not blocking on untouched filters.
- expect(screen.getByTestId(getTestId('apply-button'))).toBeDisabled();
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeEnabled();
Review Comment:
This now enables Apply on initial load even though the required default is already applied and the optional filter was never touched: retaining its empty selected mask makes `{}` differ from the absent applied entry. Could we normalize that no-op state and keep the no-pending assertion disabled?
--
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): preserve selected filter state [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4172167736
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -391,12 +391,9 @@ const FilterBar: FC<FiltersBarProps> = ({
}
});
- // Remove stale entries that no longer exist in dataMaskApplied
+ // Remove stale entries that no longer exist in the configured filters
Object.keys(updated).forEach(filterId => {
Review Comment:
Disabling Enable cross-filtering clears the Redux masks via `clearDataMaskState()`, but the native filters remain configured, so this now retains the old time-range label while charts run without that predicate; the next Apply restores it. Could explicit applied-state resets also reset the selected state?
--
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): preserve selected filter state [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #44803:
URL: https://github.com/apache/superset/pull/44803#discussion_r4173860920
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1433,202 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar preserves a selected time range when its applied data mask is removed', async () => {
+ setupTimeRangeMocks();
+
+ mockedFetchTimeRange.mockResolvedValue({
+ value: '2021-04-07T00:00:00 β€ ds < 2021-04-14T00:00:00',
+ });
+
+ fetchMock.post('glob:*/api/v1/chart/data', {
+ result: [
+ {
+ data: [{ ds: '2021-04-14T00:00:00' }],
+ colnames: ['ds'],
+ coltypes: [2],
+ applied_filters: [],
+ },
+ ],
+ });
+
+ const filterId = 'NATIVE_FILTER-keep-time-range';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+
+ const filter = createFilter({
+ id: filterId,
+ name: 'Time range',
+ filterType: 'filter_time',
+ targets: [{ datasetId: 7, column: { name: 'ds' } }],
+ defaultDataMask: {
+ filterState: { value: 'Last week' },
+ extraFormData: { time_range: 'Last week' },
+ },
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, 'Last week', {
+ time_range: 'Last week',
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Horizontal,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(<FilterBar orientation={FilterBarOrientation.Horizontal} />, {
+ store,
+ useDnd: true,
+ useRouter: true,
+ });
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ await waitFor(() => {
+ expect(screen.queryByTestId('loading-indicator')).not.toBeInTheDocument();
+ });
+
+ expect(screen.getByText('Last week')).toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Last week'));
+
+ const rangeType = screen.getByLabelText('Range type');
+ await userEvent.click(rangeType);
+ await userEvent.click(screen.getByText('Current'));
+
+ await userEvent.click(screen.getByRole('radio', { name: 'Current month' }));
+
+ const timeFilterApply = screen.getByTestId(
+ 'date-filter-control__apply-button',
+ );
+ expect(timeFilterApply).not.toBeNull();
+ expect(timeFilterApply).not.toBeDisabled();
+
+ await userEvent.click(timeFilterApply!);
+
+ const filterBarApply = screen.getByTestId(getTestId('apply-button'));
+ expect(filterBarApply).toBeEnabled();
+
+ await userEvent.click(filterBarApply);
+
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(
+ filterId,
+ expect.objectContaining({
+ filterState: expect.objectContaining({
+ value: 'Current month',
+ }),
+ extraFormData: {
+ time_range: 'Current month',
+ },
+ }),
+ );
+
+ expect((store.getState() as typeof state).dataMask[filterId]).toEqual(
+ expect.objectContaining({
+ filterState: expect.objectContaining({
+ value: 'Current month',
+ }),
+ extraFormData: {
+ time_range: 'Current month',
+ },
+ }),
+ );
+
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+
+ await act(async () => {
+ store.dispatch(dataMaskActions.removeDataMask(filterId));
+ jest.advanceTimersByTime(300);
+ });
+
+ expect(screen.getByText('Current month')).toBeInTheDocument();
+
+ const clearAllButton = screen.getByText('Clear all');
+ await userEvent.click(clearAllButton);
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ expect(screen.getByTestId(getTestId('apply-button'))).toBeEnabled();
+
+ await userEvent.click(screen.getByTestId(getTestId('apply-button')));
+
+ expect(updateDataMaskSpy).toHaveBeenLastCalledWith(
+ filterId,
+ expect.objectContaining({
+ id: filterId,
+ filterState: {
+ value: undefined,
+ validateStatus: undefined,
+ },
+ extraFormData: {},
+ }),
+ );
+
+ updateDataMaskSpy.mockRestore();
+});
+
+test('FilterBar clears selected state when cross-filtering is disabled', async () => {
Review Comment:
Running this reset test alone leaves `fetchTimeRange` as a bare mock, so `DateFilterLabel` calls `.then` on `undefined` before reaching the reset assertions; the whole-file run inherits the preceding testβs implementation. Could this test initialize its own promise-returning mock?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -369,9 +373,13 @@ const FilterBar: FC<FiltersBarProps> = ({
useEffect(() => {
const dashboardChanged = dashboardId !== previousDashboardId;
+ const crossFiltersDisabled =
+ previousCrossFiltersEnabled === true && crossFiltersEnabled === false;
if (dashboardChanged) {
setDataMaskSelected(() => dataMaskApplied);
+ } else if (crossFiltersDisabled) {
+ setDataMaskSelected(() => dataMaskApplied);
Review Comment:
A failed attempt to enable cross-filtering rolls the setting back from true to false without clearing the applied masks, so this replaces any pending native-filter selection with the old applied value. Could this reset be tied to an actual mask clear rather than every true β false transition?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx:
##########
@@ -1411,3 +1432,147 @@ test('FilterBar with orientation=Vertical renders Vertical layout (sanity counte
screen.queryByRole('img', { name: 'setting' }),
).not.toBeInTheDocument();
});
+
+test('FilterBar preserves a selected time range when its applied data mask is removed', async () => {
+ setupTimeRangeMocks();
+
+ mockedFetchTimeRange.mockResolvedValue({
+ value: '2021-04-07T00:00:00 β€ ds < 2021-04-14T00:00:00',
+ });
+
+ fetchMock.post('glob:*/api/v1/chart/data', {
+ result: [
+ {
+ data: [{ ds: '2021-04-14T00:00:00' }],
+ colnames: ['ds'],
+ coltypes: [2],
+ applied_filters: [],
+ },
+ ],
+ });
+
+ const filterId = 'NATIVE_FILTER-keep-time-range';
+ const updateDataMaskSpy = jest.spyOn(dataMaskActions, 'updateDataMask');
+
+ const filter = createFilter({
+ id: filterId,
+ name: 'Time range',
+ filterType: 'filter_time',
+ targets: [{ datasetId: 7, column: { name: 'ds' } }],
+ defaultDataMask: {
+ filterState: { value: 'Last week' },
+ extraFormData: { time_range: 'Last week' },
+ },
+ chartsInScope: [18],
+ });
+
+ const state = createStateWithFilter(
+ filter,
+ createDataMask(filterId, 'Last week', {
+ time_range: 'Last week',
+ }),
+ {
+ filterBarOrientation: FilterBarOrientation.Horizontal,
+ metadata: {
+ native_filter_configuration: [filter],
+ chart_configuration: {},
+ },
+ },
+ );
+
+ const store = createStore(state, reducerIndex);
+
+ render(<FilterBar orientation={FilterBarOrientation.Horizontal} />, {
+ store,
+ useDnd: true,
+ useRouter: true,
+ });
+
+ await act(async () => {
+ jest.advanceTimersByTime(1000);
+ });
+
+ await waitFor(() => {
+ expect(screen.queryByTestId('loading-indicator')).not.toBeInTheDocument();
+ });
+
+ expect(screen.getByText('Last week')).toBeInTheDocument();
+
+ await userEvent.click(screen.getByText('Last week'));
+
+ const rangeType = screen.getByLabelText('Range type');
+ await userEvent.click(rangeType);
+ await userEvent.click(screen.getByText('Current'));
+
+ await userEvent.click(screen.getByRole('radio', { name: 'Current month' }));
+
+ const timeFilterApply = screen.getByTestId(
+ 'date-filter-control__apply-button',
+ );
+ expect(timeFilterApply).not.toBeNull();
+ expect(timeFilterApply).not.toBeDisabled();
+
+ await userEvent.click(timeFilterApply!);
+
+ const filterBarApply = screen.getByTestId(getTestId('apply-button'));
+ expect(filterBarApply).toBeEnabled();
+
+ await userEvent.click(filterBarApply);
+
+ expect(updateDataMaskSpy).toHaveBeenCalledWith(
+ filterId,
+ expect.objectContaining({
+ filterState: expect.objectContaining({
+ value: 'Current month',
+ }),
+ extraFormData: {
+ time_range: 'Current month',
+ },
+ }),
+ );
+
+ expect((store.getState() as RootState).dataMask[filterId]).toEqual(
Review Comment:
The current head still fails frontend type-checking: `typeof state` triggers TS2352 at line 1535, and the new reset test adds TS2339 for `crossFiltersEnabled` and `dataMask` at lines 1610 and 1632. Could we type both fixtures and state reads so the frontend type-check passes?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx:
##########
@@ -391,12 +391,9 @@ const FilterBar: FC<FiltersBarProps> = ({
}
});
- // Remove stale entries that no longer exist in dataMaskApplied
+ // Remove stale entries that no longer exist in the configured filters
Object.keys(updated).forEach(filterId => {
Review Comment:
The toggle reset is now handled, but disabling cross-filtering, applying a time range from chat, then clicking the toastβs Undo still leaves that range displayed: Undo removes the previously absent mask without changing the setting, and Apply restores it. Could explicit Undo removals reset the selected state too?
--
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