fix(frontend): resolve race condition in DatasetUsageTab pagination s… (#35691)

Co-authored-by: Claude <noreply@anthropic.com>

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,250 @@ 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();
+});
+
+test('cleans up animation frame on unmount during loading', async () => {
+  const cancelAnimationFrameSpy = jest.spyOn(window, 'cancelAnimationFrame');
+
+  let resolvePromise: (value: any) => void;
+  const delayedPromise = new Promise(resolve => {
+    resolvePromise = resolve;
+  });
+
+  const mockOnFetchCharts = jest.fn(() => delayedPromise);
+
+  const { unmount } = 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();
+  });
+
+  // Resolve promise to trigger scroll effect
+  resolvePromise!({
+    charts: mockChartsResponse.result,
+    count: 100,
+    ids: [1, 2],
+  });
+
+  // Wait for loading to complete (which queues requestAnimationFrame)
+  await waitFor(() => {
+    expect(screen.queryByLabelText('Loading')).not.toBeInTheDocument();
+  });
+
+  // Unmount before animation frame fires
+  unmount();
+
+  // Cleanup should have cancelled the animation frame
+  expect(cancelAnimationFrameSpy).toHaveBeenCalled();
+
+  cancelAnimationFrameSpy.mockRestore();
+});

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],
   );