(superset) 01/01: fix(frontend): resolve race condition in DatasetUsageTab pagination scroll

[jl...@apache.org]

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 <jo...@preset.io>
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 <no...@anthropic.com>
---
 .../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],
   );