This is an automated email from the ASF dual-hosted git repository. sadpandajoe pushed a commit to branch fix/filter-removal-timer-cleanup in repository https://gitbox.apache.org/repos/asf/superset.git
commit b4f1ccb0eb6f440a52a7eb4c55276f1cb1682d6a Author: Joe Li <[email protected]> AuthorDate: Wed Sep 30 15:36:06 2026 -0700 fix(native-filters): keep pending removal timers when another filter is removed useItemStateManager cleared the finalization timer of every pending removal whenever removedItems changed, so removing a second filter within 5s left the first stuck in the pending state. Clear pending timers only on unmount and on resetState instead. Co-Authored-By: Claude Sonnet 5.5 <[email protected]> --- .../hooks/useFilterOperations.test.ts | 106 +++++++++++++++++++++ .../hooks/useItemStateManager.ts | 31 +++--- 2 files changed, 125 insertions(+), 12 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts index 87a6d475341..638e6c6241b 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useFilterOperations.test.ts @@ -256,3 +256,109 @@ test('restoreFilter cancels the pending removal before the delay elapses', () => jest.useRealTimers(); }); + +function useFilterOperationsWithStateManager( + filterIds: string[], + form: FormInstance<NativeFiltersForm>, +) { + const filterState = useItemStateManager(filterIds, {}); + const filterOperations = useFilterOperations({ + form, + filterState, + filterIds, + filterConfigMap: {}, + handleModifyItem: jest.fn(), + setActiveItem: jest.fn(), + setSaveAlertVisible: jest.fn(), + }); + return { filterState, filterOperations }; +} + +test('removing a second filter does not cancel the first filter’s pending finalization', () => { + jest.useFakeTimers(); + const form = { + getFieldValue: () => undefined, + } as unknown as FormInstance<NativeFiltersForm>; + const { result } = renderHook(() => + useFilterOperationsWithStateManager(['f1', 'f2'], form), + ); + + act(() => { + result.current.filterOperations.handleRemoveFilter('f1'); + }); + act(() => { + jest.advanceTimersByTime(1000); + }); + act(() => { + result.current.filterOperations.handleRemoveFilter('f2'); + }); + // f1's 5s delay elapses 4s after f2 was removed + act(() => { + jest.advanceTimersByTime(4000); + }); + expect(result.current.filterState.removedItems.f1).toEqual({ + isPending: false, + }); + expect(result.current.filterState.removedItems.f2).toEqual( + expect.objectContaining({ isPending: true }), + ); + + act(() => { + jest.advanceTimersByTime(1000); + }); + expect(result.current.filterState.removedItems.f2).toEqual({ + isPending: false, + }); + + jest.useRealTimers(); +}); + +test('resetState cancels pending removal timers so they cannot repopulate the cleared state', () => { + jest.useFakeTimers(); + const form = { + getFieldValue: () => undefined, + } as unknown as FormInstance<NativeFiltersForm>; + const { result } = renderHook(() => + useFilterOperationsWithStateManager(['f1'], form), + ); + + act(() => { + result.current.filterOperations.handleRemoveFilter('f1'); + }); + act(() => { + result.current.filterState.resetState(); + }); + expect(result.current.filterState.removedItems).toEqual({}); + + act(() => { + jest.advanceTimersByTime(5000); + }); + expect(result.current.filterState.removedItems).toEqual({}); + + jest.useRealTimers(); +}); + +test('unmounting cancels pending removal timers', () => { + jest.useFakeTimers(); + const form = { + getFieldValue: () => undefined, + } as unknown as FormInstance<NativeFiltersForm>; + const { result, unmount } = renderHook(() => + useFilterOperationsWithStateManager(['f1'], form), + ); + + act(() => { + result.current.filterOperations.handleRemoveFilter('f1'); + }); + const { timerId } = result.current.filterState.removedItems.f1 as { + timerId: number; + }; + const clearTimeoutSpy = jest.spyOn(window, 'clearTimeout'); + + unmount(); + + expect(clearTimeoutSpy).toHaveBeenCalledWith(timerId); + + clearTimeoutSpy.mockRestore(); + jest.useRealTimers(); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useItemStateManager.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useItemStateManager.ts index de783d13782..6cb84c6979c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useItemStateManager.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/hooks/useItemStateManager.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useState, useCallback, useEffect } from 'react'; +import { useState, useCallback, useEffect, useRef } from 'react'; import { uniq, isEmpty } from 'lodash-es'; import { FilterChangesType, FilterRemoval } from '../types'; @@ -75,13 +75,27 @@ export function useItemStateManager( const [orderedIds, setOrderedIds] = useState<string[]>(initialOrder); const [renderedIds, setRenderedIds] = useState<string[]>(DEFAULT_EMPTY_ARRAY); + const removedItemsRef = useRef(removedItems); + useEffect(() => { + removedItemsRef.current = removedItems; + }, [removedItems]); + + const clearPendingRemovalTimers = useCallback(() => { + Object.values(removedItemsRef.current).forEach(removal => { + if (removal?.isPending && removal.timerId) { + clearTimeout(removal.timerId); + } + }); + }, []); + const resetState = useCallback(() => { + clearPendingRemovalTimers(); setChanges(DEFAULT_CHANGES); setNewIds(DEFAULT_EMPTY_ARRAY); setRemovedItems(DEFAULT_REMOVED_ITEMS); setErroredIds(DEFAULT_EMPTY_ARRAY); setRenderedIds(DEFAULT_EMPTY_ARRAY); - }, []); + }, [clearPendingRemovalTimers]); const addToRendered = useCallback((id: string) => { setRenderedIds(prev => { @@ -124,16 +138,9 @@ export function useItemStateManager( }); }, [configMap, newIds, removedItems]); - useEffect( - () => () => { - Object.values(removedItems).forEach(removal => { - if (removal?.isPending && removal.timerId) { - clearTimeout(removal.timerId); - } - }); - }, - [removedItems], - ); + // Only on unmount: clearing on every removedItems change would cancel the + // timers of earlier, still-pending removals whenever another one is added. + useEffect(() => clearPendingRemovalTimers, [clearPendingRemovalTimers]); return { changes,
