This is an automated email from the ASF dual-hosted git repository. jli pushed a commit to branch fix-dataset-usage-async in repository https://gitbox.apache.org/repos/asf/superset.git
commit 7cb33c205634f4fa1fce8c7f5979413b2c123554 Author: Joe Li <[email protected]> AuthorDate: Thu Oct 16 15:30:58 2025 -0700 fix(frontend): resolve race condition in DatasetUsageTab pagination scroll The DatasetUsageTab component had a race condition where the scroll-to-top action occurred before paginated data finished loading. This resulted in poor UX on slow networks, with users seeing old data briefly before it updated. Changes: - Added prevLoadingRef to track loading state transitions - Implemented useEffect that triggers scroll when loading completes (true → false) - Removed arbitrary 100ms setTimeout from handlePageChange - Uses requestAnimationFrame for precise DOM timing - Added 6 comprehensive RTL tests for async pagination behavior - Fixed test isolation by properly mocking/restoring Element.prototype.scrollTo All 15 tests passing (6 new async tests added). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../DatasetUsageTab/DatasetUsageTab.test.tsx | 215 +++++++++++++++++++++ .../components/DatasetUsageTab/index.tsx | 27 ++- 2 files changed, 237 insertions(+), 5 deletions(-) diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/DatasetUsageTab.test.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/DatasetUsageTab.test.tsx index 592abfe495..76c41415a5 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/DatasetUsageTab.test.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/DatasetUsageTab.test.tsx @@ -22,6 +22,7 @@ import { screen, waitFor, } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; import fetchMock from 'fetch-mock'; import DatasetUsageTab from '.'; @@ -109,13 +110,25 @@ const setupTest = (props = {}) => { }); }; +// Store original scrollTo to restore after tests +let originalScrollTo: typeof Element.prototype.scrollTo; + +beforeAll(() => { + // Capture original scrollTo implementation once before all tests + originalScrollTo = Element.prototype.scrollTo; +}); + beforeEach(() => { fetchMock.reset(); jest.clearAllMocks(); + // Mock scrollTo for all tests + Element.prototype.scrollTo = jest.fn(); }); afterEach(() => { fetchMock.restore(); + // Restore original scrollTo implementation after each test + Element.prototype.scrollTo = originalScrollTo; }); test('renders empty state when no charts provided', () => { @@ -212,3 +225,205 @@ test('enables sorting for Chart and Last modified columns', async () => { expect(dashboardHeader).not.toHaveClass('ant-table-column-has-sorters'); }); }); + +test('shows loading state during pagination fetch', async () => { + let resolvePromise: (value: any) => void; + const delayedPromise = new Promise(resolve => { + resolvePromise = resolve; + }); + + const mockOnFetchCharts = jest.fn(() => delayedPromise); + + // Start with multiple pages + setupTest({ + onFetchCharts: mockOnFetchCharts, + totalCount: 100, + }); + + // Initial render - no loading + expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument(); + + // Find next page button + const nextButton = screen.getByTitle('Next Page'); + + await userEvent.click(nextButton); + + // Should show loading state + await waitFor(() => { + expect(screen.getByLabelText('Loading')).toBeInTheDocument(); + }); + + // Resolve the promise + resolvePromise!({ + charts: mockChartsResponse.result, + count: 100, + ids: [1, 2], + }); + + // Loading should disappear + await waitFor(() => { + expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument(); + }); +}); + +test('calls onFetchCharts with correct pagination parameters', async () => { + const mockOnFetchCharts = jest.fn(() => + Promise.resolve({ + charts: mockChartsResponse.result, + count: 100, + ids: [1, 2], + }), + ); + + setupTest({ + onFetchCharts: mockOnFetchCharts, + totalCount: 100, + }); + + const nextButton = screen.getByTitle('Next Page'); + + await userEvent.click(nextButton); + + await waitFor(() => { + expect(mockOnFetchCharts).toHaveBeenCalledWith( + 2, // page + 25, // pageSize + 'changed_on_delta_humanized', // sortColumn + 'desc', // sortDirection + ); + }); +}); + +test('shows error toast when fetch fails', async () => { + const mockOnFetchCharts = jest.fn(() => + Promise.reject(new Error('Network error')), + ); + const mockAddDangerToast = jest.fn(); + + setupTest({ + onFetchCharts: mockOnFetchCharts, + addDangerToast: mockAddDangerToast, + totalCount: 100, + }); + + const nextButton = screen.getByTitle('Next Page'); + + await userEvent.click(nextButton); + + await waitFor(() => { + expect(mockAddDangerToast).toHaveBeenCalledWith('Error fetching charts'); + }); + + // Loading state should be cleared + expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument(); +}); + +test('handles slow network without race condition', async () => { + let resolvePromise: (value: any) => void; + const slowPromise = new Promise(resolve => { + resolvePromise = resolve; + }); + + const mockOnFetchCharts = jest.fn(() => slowPromise); + + setupTest({ + onFetchCharts: mockOnFetchCharts, + totalCount: 100, + }); + + const nextButton = screen.getByTitle('Next Page'); + + await userEvent.click(nextButton); + + // Should be loading + await waitFor(() => { + expect(screen.getByLabelText('Loading')).toBeInTheDocument(); + }); + + // Should still be loading (data hasn't arrived) + expect(screen.getByLabelText('Loading')).toBeInTheDocument(); + + // Now resolve the promise + resolvePromise!({ + charts: mockChartsResponse.result, + count: 100, + ids: [1, 2], + }); + + // Wait for loading to complete + await waitFor(() => { + expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument(); + }); +}); + +test('scrolls to top after data loads, not before', async () => { + // Use the global scrollTo mock + const scrollToMock = Element.prototype.scrollTo as jest.Mock; + + let resolvePromise: (value: any) => void; + const delayedPromise = new Promise(resolve => { + resolvePromise = resolve; + }); + + const mockOnFetchCharts = jest.fn(() => delayedPromise); + + setupTest({ + onFetchCharts: mockOnFetchCharts, + totalCount: 100, + }); + + const nextButton = screen.getByTitle('Next Page'); + + await userEvent.click(nextButton); + + // Should be loading + await waitFor(() => { + expect(screen.getByLabelText('Loading')).toBeInTheDocument(); + }); + + // Scroll should NOT have been called yet (data still loading) + expect(scrollToMock).not.toHaveBeenCalled(); + + // Resolve the promise + resolvePromise!({ + charts: mockChartsResponse.result, + count: 100, + ids: [1, 2], + }); + + // Wait for loading to complete + await waitFor(() => { + expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument(); + }); + + // Scroll should be called after data loads (with requestAnimationFrame) + await waitFor(() => { + expect(scrollToMock).toHaveBeenCalledWith({ + top: 0, + behavior: 'smooth', + }); + }); +}); + +test('does not scroll on initial mount, only on page change', async () => { + // Use the global scrollTo mock + const scrollToMock = Element.prototype.scrollTo as jest.Mock; + + const mockOnFetchCharts = jest.fn(() => + Promise.resolve({ + charts: mockChartsResponse.result, + count: 2, + ids: [1, 2], + }), + ); + + setupTest({ onFetchCharts: mockOnFetchCharts }); + + // Wait for initial render + await waitFor(() => { + expect(screen.getByText('Test Chart 1')).toBeInTheDocument(); + }); + + // Scroll should not have been called on mount + expect(scrollToMock).not.toHaveBeenCalled(); +}); diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx index c1808dd77d..a3a1244895 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/components/DatasetUsageTab/index.tsx @@ -99,6 +99,7 @@ const DatasetUsageTab = ({ }: DatasetUsageTabProps) => { const addDangerToastRef = useRef(addDangerToast); const tableContainerRef = useRef<HTMLDivElement>(null); + const prevLoadingRef = useRef(false); const [loading, setLoading] = useState(false); const [currentPage, setCurrentPage] = useState(1); @@ -132,11 +133,13 @@ const DatasetUsageTab = ({ addDangerToastRef.current = addDangerToast; }, [addDangerToast]); - const handlePageChange = useCallback( - (page: number) => { - handleFetchCharts(page); + // Scroll to top after data loads (when loading changes from true to false) + useEffect(() => { + let frameId: number | undefined; - setTimeout(() => { + if (prevLoadingRef.current && !loading) { + // Loading just finished, scroll to top + frameId = requestAnimationFrame(() => { const tableBody = tableContainerRef.current?.querySelector('.ant-table-body'); if (tableBody) { @@ -145,7 +148,21 @@ const DatasetUsageTab = ({ behavior: 'smooth', }); } - }, 100); + }); + } + prevLoadingRef.current = loading; + + // Cleanup: cancel animation frame if component unmounts + return () => { + if (frameId !== undefined) { + cancelAnimationFrame(frameId); + } + }; + }, [loading]); + + const handlePageChange = useCallback( + (page: number) => { + handleFetchCharts(page); }, [handleFetchCharts], );
