Compare commits

...

8 Commits

Author SHA1 Message Date
Joe Li
31dcc1d954 fix(dataset): add mount guards to DatasourceModal async save operation
Prevents async state updates after component unmount in the modal's save flow by adding isMountedRef pattern to guard all operations that execute after async API calls.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-03 21:37:02 -07:00
Joe Li
eea7402e4e fix(dataset): add comprehensive mount guards to DatasourceEditor async methods
Extends the isMountedRef pattern to all async methods in DatasourceEditor
to prevent state updates and toast notifications after component unmount.

**Methods Protected**:
- onQueryFormat: Guards SQL formatting state updates and toasts
- formatSql: Guards SQL formatting via SupersetClient.post
- syncMetadata: Guards metadata sync state updates and toasts
- fetchUsageData: Guards usage data state updates and toasts (already had partial guards)

**Pattern Applied**:
- Initialize: this.isComponentMounted = false (constructor)
- Set true: componentDidMount
- Set false: componentWillUnmount
- Guard all post-await state updates and toast calls with if (this.isComponentMounted)

**Why This Matters**:
The parent DatasourceEditor contains the child DatasetUsageTab. Both components
make async calls that can resolve after unmount:
- Parent: SupersetClient.get, formatQuery, fetchSyncedColumns
- Child: onFetchCharts (calls parent's fetchUsageData)

Both need mount guards because they manage independent state. This commit
completes the pattern in the parent, matching the fix applied to the child
in the previous commit.

**Benefits**:
- Eliminates potential intermittent test failures in parent component
- Prevents memory leaks and stale state updates
- Consistent async safety pattern across parent and child
- Production safety for all SQL formatting and metadata operations

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-03 16:58:32 -07:00
Joe Li
813d184ea2 fix(dataset): prevent async state updates after unmount in DatasetUsageTab
Fixes intermittent test error: "The document global was defined when React
was initialized, but is not defined anymore."

**Root Cause**:
handleFetchCharts awaited onFetchCharts() then unconditionally updated state
(setCurrentPage, setSortColumn, setSortDirection, setLoading). If the component
unmounted before the Promise resolved, these updates would execute on an
unmounted component, causing the jsdom crash.

**Fix**:
- Added isMountedRef to track component mount state
- Guard all post-await state updates with isMountedRef.current checks
- Set isMountedRef.current = true on mount (handles React Strict Mode double-mount)
- Set isMountedRef.current = false on unmount to prevent stale updates

**Test Changes**:
- Updated mock to simulate Table wrapper's pagination override behavior
- Mock now merges recordCount → total with pagination prop
- Ensures tests validate actual runtime behavior

**Benefits**:
- Eliminates intermittent test failures
- Prevents memory leaks in production
- React Strict Mode compatible
- Better UX (no stale state updates if user navigates during fetch)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-03 15:47:06 -07:00
Joe Li
13553a7205 fix(dataset): add pagination props to DatasetUsageTab Table component
Fixes pagination display issue where navigating to page 2+ would show
incorrect page counts (e.g., "1 page" instead of "3 pages").

**Root cause**: Table component was missing `current`, `total`, and
`pageSize` props in the pagination config, causing it to calculate
total pages from `data.length` instead of `totalCount`.

**Changes**:
- Add `current`, `total`, and `pageSize` to pagination config
- Update test assertions from `aria-current` to `ant-pagination-item-active`
- Update test queries from `getByRole('link')` to `getByRole('listitem')`
  (Ant Design 5 renders pagination items as <li>, not <a>)
- Add TypeScript type guard for pagination prop access
- Fix type compatibility for pagination.onChange (undefined → null)

**Test results**: All 16 tests passing (pagination navigation, state
persistence, edge cases, and async cleanup tests)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-03 10:17:27 -07:00
Joe Li
ecce53e12f test(dataset): add comprehensive tests for DatasetUsageTab pagination and async cleanup
Add TDD tests to verify:
- Timeout cleanup on component unmount (prevents async updates after unmount)
- Rapid pagination click handling (clears pending timeouts)
- Scroll behavior after pagination
- Pagination state across multiple page changes
- Edge cases (single page, partial pages)

Tests currently fail as expected, exposing bugs:
- Missing `current` and `total` in pagination config
- Unmanaged setTimeout causing race conditions

Improvements:
- Use typed TableProps instead of `any` for mock
- Use aria-current instead of Ant Design class names for robustness
- Use userEvent for realistic user interactions
- Combine DOM assertions with soft prop assertions

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-02 22:30:20 -07:00
Joe Li
bd35faa8a0 style: fix eslint arrow-body-style warning in DatasetUsageTab
Move eslint-disable comment to correct line to suppress arrow-body-style
warning for useEffect cleanup function.
2025-10-02 22:30:20 -07:00
Joe Li
69879986e6 fix(dataset): add timeout cleanup in DatasetUsageTab to prevent async updates after unmount
Add proper setTimeout cleanup using useRef + useEffect to prevent intermittent
"document global was defined..." errors during test execution.

Component changes:
- Add scrollTimeoutRef to track timeout IDs with proper TypeScript typing
- Add cleanup useEffect to clear timeout on component unmount
- Update handlePageChange to clear pending timeouts before setting new ones
- Set scrollTimeoutRef to null after timeout executes

Test changes:
- Add Table component mock to capture onChange handlers for pagination tests
- Add 3 comprehensive test cases validating timeout cleanup:
  * Cleanup on unmount prevents async updates after component removal
  * Rapid pagination clears pending timeouts (only last scroll executes)
  * Normal pagination scrolls to top with 100ms delay
- Use fake timers with proper cleanup in afterEach
- Mock Element.prototype.scrollTo for JSDOM compatibility

Benefits:
- Prevents memory leaks from lingering timeouts
- Prevents stale DOM references in production
- Fixes test flakiness from async callback race conditions
- Prevents duplicate scroll operations on rapid pagination clicks

All tests passing (12/12). No intermittent errors when running with
ExploreChartPanel tests (20/20 total).

Fixes #34707

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-02 22:30:20 -07:00
Joe Li
41fd703a8c test(dataset): add TDD tests for async timeout cleanup in DatasetUsageTab
Add comprehensive test coverage for setTimeout cleanup to prevent
intermittent "document global was defined..." errors during test execution.

Test improvements:
- Add scrollTo mock lifecycle management (save/restore in beforeEach/afterEach)
- Add 3 TDD test cases for timeout cleanup scenarios:
  * Cleanup on unmount to prevent async updates
  * Clear pending timeout on rapid pagination clicks
  * Scroll to top after pagination with delay
- Use RTL-compliant getByRole('link') for pagination interaction
- Optimize mock reuse via Element.prototype.scrollTo reference
- Centralize timer cleanup in afterEach for guaranteed reset

Component changes:
- Update Table pagination to use recordCount/usePagination pattern
- Align with @superset-ui/core Table component API

Tests are currently failing (TDD) - component fix pending.

Related to #34707

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-02 22:30:20 -07:00
4 changed files with 646 additions and 36 deletions

View File

@@ -16,7 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/
import { FunctionComponent, useState, useEffect, useCallback } from 'react';
import {
FunctionComponent,
useState,
useEffect,
useCallback,
useRef,
} from 'react';
import { useSelector } from 'react-redux';
import {
styled,
@@ -108,6 +114,14 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
const [isSaving, setIsSaving] = useState(false);
const [isEditing, setIsEditing] = useState<boolean>(false);
const [modal, contextHolder] = Modal.useModal();
const isMountedRef = useRef(true);
useEffect(() => {
isMountedRef.current = true;
return () => {
isMountedRef.current = false;
};
}, []);
const buildPayload = (datasource: Record<string, any>) => {
const payload: Record<string, any> = {
table_name: datasource.table_name,
@@ -185,6 +199,9 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
};
const onConfirmSave = async () => {
// Pull out extra fields into the extra object
if (!isMountedRef.current) {
return;
}
setIsSaving(true);
try {
await SupersetClient.put({
@@ -195,6 +212,10 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
const { json } = await SupersetClient.get({
endpoint: `/api/v1/dataset/${currentDatasource?.id}`,
});
if (!isMountedRef.current) {
return;
}
setIsSaving(false);
addSuccessToast(t('The dataset has been saved'));
// eslint-disable-next-line no-param-reassign
json.result.type = 'table';
@@ -204,8 +225,11 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
});
onHide();
} catch (response) {
setIsSaving(false);
const error = await getClientErrorObject(response);
if (!isMountedRef.current) {
return;
}
setIsSaving(false);
let errorResponse: SupersetError | undefined;
let errorText: string | undefined;
// sip-40 error response

View File

@@ -622,6 +622,7 @@ const ResultTable =
class DatasourceEditor extends PureComponent {
constructor(props) {
super(props);
this.isComponentMounted = false;
this.state = {
datasource: {
...props.datasource,
@@ -760,11 +761,17 @@ class DatasourceEditor extends PureComponent {
try {
const response = await this.props.formatQuery(datasource.sql);
if (!this.isComponentMounted) {
return;
}
this.onDatasourcePropChange('sql', response.json.result);
this.props.addSuccessToast(t('SQL was formatted'));
} catch (error) {
const { error: clientError, statusText } =
await getClientErrorObject(error);
if (!this.isComponentMounted) {
return;
}
this.props.addDangerToast(
clientError ||
statusText ||
@@ -808,11 +815,17 @@ class DatasourceEditor extends PureComponent {
body: JSON.stringify({ sql: datasource.sql }),
headers: { 'Content-Type': 'application/json' },
});
if (!this.isComponentMounted) {
return;
}
this.onDatasourcePropChange('sql', response.json.result);
this.props.addSuccessToast(t('SQL was formatted'));
} catch (error) {
const { error: clientError, statusText } =
await getClientErrorObject(error);
if (!this.isComponentMounted) {
return;
}
this.props.addDangerToast(
clientError ||
statusText ||
@@ -823,28 +836,41 @@ class DatasourceEditor extends PureComponent {
async syncMetadata() {
const { datasource } = this.state;
if (!this.isComponentMounted) {
return;
}
this.setState({ metadataLoading: true });
try {
const newCols = await fetchSyncedColumns(datasource);
if (!this.isComponentMounted) {
return;
}
const columnChanges = updateColumns(
datasource.columns,
newCols,
this.props.addSuccessToast,
);
if (!this.isComponentMounted) {
return;
}
this.setColumns({
databaseColumns: columnChanges.finalColumns.filter(
col => !col.expression, // remove calculated columns
),
});
this.props.addSuccessToast(t('Metadata has been synced'));
this.setState({ metadataLoading: false });
if (this.isComponentMounted) {
this.props.addSuccessToast(t('Metadata has been synced'));
this.setState({ metadataLoading: false });
}
} catch (error) {
const { error: clientError, statusText } =
await getClientErrorObject(error);
this.props.addDangerToast(
clientError || statusText || t('An error has occurred'),
);
this.setState({ metadataLoading: false });
if (this.isComponentMounted) {
this.props.addDangerToast(
clientError || statusText || t('An error has occurred'),
);
this.setState({ metadataLoading: false });
}
}
}
@@ -901,10 +927,12 @@ class DatasourceEditor extends PureComponent {
id: ids[index],
}));
this.setState({
usageCharts: chartsWithIds,
usageChartsCount: json?.count || 0,
});
if (this.isComponentMounted) {
this.setState({
usageCharts: chartsWithIds,
usageChartsCount: json?.count || 0,
});
}
return {
charts: chartsWithIds,
@@ -914,15 +942,17 @@ class DatasourceEditor extends PureComponent {
} catch (error) {
const { error: clientError, statusText } =
await getClientErrorObject(error);
this.props.addDangerToast(
clientError ||
statusText ||
t('An error occurred while fetching usage data'),
);
this.setState({
usageCharts: [],
usageChartsCount: 0,
});
if (this.isComponentMounted) {
this.props.addDangerToast(
clientError ||
statusText ||
t('An error occurred while fetching usage data'),
);
this.setState({
usageCharts: [],
usageChartsCount: 0,
});
}
return {
charts: [],
count: 0,
@@ -1883,6 +1913,7 @@ class DatasourceEditor extends PureComponent {
}
componentDidMount() {
this.isComponentMounted = true;
Mousetrap.bind('ctrl+shift+f', e => {
e.preventDefault();
if (this.state.isEditMode) {
@@ -1894,6 +1925,7 @@ class DatasourceEditor extends PureComponent {
}
componentWillUnmount() {
this.isComponentMounted = false;
Mousetrap.unbind('ctrl+shift+f');
this.props.resetQuery();
}

View File

@@ -17,14 +17,149 @@
* under the License.
*/
import {
act,
createWrapper,
render,
screen,
waitFor,
} from 'spec/helpers/testing-library';
import userEvent from '@testing-library/user-event';
import fetchMock from 'fetch-mock';
import {
TableProps,
OnChangeFunction,
} from '@superset-ui/core/components/Table';
import DatasetUsageTab from '.';
interface Chart {
id?: number;
slice_name: string;
url: string;
certified_by?: string;
certification_details?: string;
description?: string;
owners: Array<{
first_name: string;
last_name: string;
id: number;
}>;
changed_on_delta_humanized: string;
changed_on?: string;
changed_by: {
first_name: string;
last_name: string;
id: number;
} | null;
dashboards: Array<{
id: number;
dashboard_title: string;
url: string;
}>;
}
jest.mock('@superset-ui/core/components/Table', () => {
const actual = jest.requireActual('@superset-ui/core/components/Table');
const ActualTable = actual.default;
let latestPaginationHandler:
| ((page: number, pageSize?: number) => void)
| null = null;
let latestPaginationPageSize: number | undefined;
let latestPaginationCurrent: number | undefined;
let latestPaginationTotal: number | undefined;
let latestOnChangeHandler: OnChangeFunction<Chart> | null = null;
const MockTable = <RecordType extends object>({
pagination,
onChange,
recordCount,
defaultPageSize = 15,
usePagination = true,
...rest
}: TableProps<RecordType>) => {
// Simulate the Table wrapper's pagination merging logic
let mergedPagination = pagination;
if (usePagination) {
// Build default pagination settings like the wrapper does
const paginationDefaults: {
hideOnSinglePage: boolean;
pageSize: number;
total?: number;
} = {
hideOnSinglePage: true,
pageSize: defaultPageSize,
};
// Add total from recordCount if provided
if (recordCount) {
paginationDefaults.total = recordCount;
}
// Merge with pagination overrides
if (pagination && typeof pagination !== 'boolean') {
mergedPagination = { ...paginationDefaults, ...pagination };
} else if (!pagination) {
mergedPagination = paginationDefaults;
}
}
// Capture the merged pagination for assertions
if (mergedPagination && typeof mergedPagination !== 'boolean') {
latestPaginationPageSize = mergedPagination.pageSize;
latestPaginationCurrent = mergedPagination.current;
latestPaginationTotal = mergedPagination.total;
latestPaginationHandler = mergedPagination.onChange || null;
}
// Cast to Chart type since we know DatasetUsageTab uses Chart
latestOnChangeHandler =
(onChange as OnChangeFunction<Chart> | undefined) || null;
return (
<ActualTable
pagination={mergedPagination}
onChange={onChange}
recordCount={recordCount}
defaultPageSize={defaultPageSize}
usePagination={usePagination}
{...rest}
/>
);
};
return {
__esModule: true,
...actual,
default: MockTable,
__getLatestPaginationHandler: () => latestPaginationHandler,
__getLatestPaginationPageSize: () => latestPaginationPageSize,
__getLatestPaginationCurrent: () => latestPaginationCurrent,
__getLatestPaginationTotal: () => latestPaginationTotal,
__getLatestOnChangeHandler: () => latestOnChangeHandler,
__resetPaginationHandler: () => {
latestPaginationHandler = null;
latestPaginationPageSize = undefined;
latestPaginationCurrent = undefined;
latestPaginationTotal = undefined;
latestOnChangeHandler = null;
},
};
});
const tableMock = jest.requireMock('@superset-ui/core/components/Table') as {
__getLatestPaginationHandler: () =>
| ((page: number, pageSize?: number) => void)
| null;
__getLatestPaginationPageSize: () => number | undefined;
__getLatestPaginationCurrent: () => number | undefined;
__getLatestPaginationTotal: () => number | undefined;
__resetPaginationHandler: () => void;
__getLatestOnChangeHandler: () =>
| ((pagination: { current: number; pageSize?: number }) => void)
| null;
};
const mockChartsResponse = {
result: [
{
@@ -83,6 +218,9 @@ const mockChartsResponse = {
ids: [1, 2],
};
// Store original scrollTo to restore after tests
let originalScrollTo: typeof Element.prototype.scrollTo;
const setupTest = (props = {}) => {
const mockOnFetchCharts = jest.fn(() =>
Promise.resolve({
@@ -101,21 +239,48 @@ const setupTest = (props = {}) => {
...props,
};
return render(<DatasetUsageTab {...defaultProps} />, {
const result = render(<DatasetUsageTab {...defaultProps} />, {
wrapper: createWrapper({
useRedux: true,
useRouter: true,
}),
});
return { ...result, mockOnFetchCharts };
};
beforeEach(() => {
fetchMock.reset();
jest.clearAllMocks();
// Save original scrollTo and mock it for JSDOM compatibility
originalScrollTo = Element.prototype.scrollTo;
Object.defineProperty(Element.prototype, 'scrollTo', {
value: jest.fn(),
configurable: true,
writable: true,
});
// eslint-disable-next-line no-underscore-dangle
tableMock.__resetPaginationHandler();
});
afterEach(() => {
fetchMock.restore();
// Restore real timers in case a test with fakeTimers failed mid-test
jest.useRealTimers();
// Restore original scrollTo to avoid test pollution
if (originalScrollTo) {
Object.defineProperty(Element.prototype, 'scrollTo', {
value: originalScrollTo,
configurable: true,
writable: true,
});
} else {
delete (Element.prototype as any).scrollTo;
}
});
test('renders empty state when no charts provided', () => {
@@ -212,3 +377,363 @@ test('enables sorting for Chart and Last modified columns', async () => {
expect(dashboardHeader).not.toHaveClass('ant-table-column-has-sorters');
});
});
test('clears scroll timeout on unmount to prevent async updates', async () => {
jest.useFakeTimers();
const scrollToMock = Element.prototype.scrollTo as jest.Mock;
scrollToMock.mockClear();
const manyCharts = Array.from({ length: 51 }, (_, i) => ({
id: i + 1,
slice_name: `Test Chart ${i + 1}`,
url: `/explore/${i + 1}/`,
owners: [],
changed_on_delta_humanized: '1 day ago',
changed_by: null,
dashboards: [],
}));
const { unmount, mockOnFetchCharts } = setupTest({
charts: manyCharts.slice(0, 25),
totalCount: 51,
});
// eslint-disable-next-line no-underscore-dangle
const onChangeHandler = tableMock.__getLatestOnChangeHandler();
// eslint-disable-next-line no-underscore-dangle
const pageSize = tableMock.__getLatestPaginationPageSize();
expect(onChangeHandler).toBeDefined();
onChangeHandler?.({ current: 2, pageSize } as {
current: number;
pageSize?: number;
});
await waitFor(() =>
expect(mockOnFetchCharts).toHaveBeenCalledWith(
2,
25,
expect.any(String),
expect.any(String),
),
);
unmount();
act(() => {
jest.runAllTimers();
});
expect(scrollToMock).not.toHaveBeenCalled();
});
test('clears pending scroll timeout on rapid pagination clicks', async () => {
jest.useFakeTimers();
const scrollToMock = Element.prototype.scrollTo as jest.Mock;
scrollToMock.mockClear();
const manyCharts = Array.from({ length: 60 }, (_, i) => ({
id: i + 1,
slice_name: `Test Chart ${i + 1}`,
url: `/explore/${i + 1}/`,
owners: [],
changed_on_delta_humanized: '1 day ago',
changed_by: null,
dashboards: [],
}));
const { mockOnFetchCharts } = setupTest({
charts: manyCharts.slice(0, 25),
totalCount: 60,
});
// eslint-disable-next-line no-underscore-dangle
const firstOnChange = tableMock.__getLatestOnChangeHandler();
// eslint-disable-next-line no-underscore-dangle
const pageSize = tableMock.__getLatestPaginationPageSize();
expect(firstOnChange).toBeDefined();
firstOnChange?.({ current: 2, pageSize } as {
current: number;
pageSize?: number;
});
await waitFor(() =>
expect(mockOnFetchCharts).toHaveBeenCalledWith(
2,
25,
expect.any(String),
expect.any(String),
),
);
act(() => {
jest.advanceTimersByTime(50);
});
// eslint-disable-next-line no-underscore-dangle
const secondOnChange = tableMock.__getLatestOnChangeHandler();
expect(secondOnChange).toBeDefined();
secondOnChange?.({ current: 3, pageSize } as {
current: number;
pageSize?: number;
});
await waitFor(() =>
expect(mockOnFetchCharts).toHaveBeenCalledWith(
3,
25,
expect.any(String),
expect.any(String),
),
);
act(() => {
jest.runAllTimers();
});
expect(scrollToMock).toHaveBeenCalledTimes(1);
expect(scrollToMock).toHaveBeenCalledWith({ top: 0, behavior: 'smooth' });
});
test('scrolls table to top after pagination with delay', async () => {
jest.useFakeTimers();
const scrollToMock = Element.prototype.scrollTo as jest.Mock;
scrollToMock.mockClear();
const manyCharts = Array.from({ length: 30 }, (_, i) => ({
id: i + 1,
slice_name: `Test Chart ${i + 1}`,
url: `/explore/${i + 1}/`,
owners: [],
changed_on_delta_humanized: '1 day ago',
changed_by: null,
dashboards: [],
}));
const { mockOnFetchCharts } = setupTest({
charts: manyCharts.slice(0, 25),
totalCount: 30,
});
// eslint-disable-next-line no-underscore-dangle
const onChangeHandler = tableMock.__getLatestOnChangeHandler();
// eslint-disable-next-line no-underscore-dangle
const pageSize = tableMock.__getLatestPaginationPageSize();
expect(onChangeHandler).toBeDefined();
onChangeHandler?.({ current: 2, pageSize } as {
current: number;
pageSize?: number;
});
await waitFor(() =>
expect(mockOnFetchCharts).toHaveBeenCalledWith(
2,
25,
expect.any(String),
expect.any(String),
),
);
expect(scrollToMock).not.toHaveBeenCalled();
act(() => {
jest.advanceTimersByTime(100);
});
expect(scrollToMock).toHaveBeenCalledWith({
top: 0,
behavior: 'smooth',
});
});
test('displays correct pagination and navigates between pages', async () => {
const manyCharts = Array.from({ length: 30 }, (_, i) => ({
id: i + 1,
slice_name: `Test Chart ${i + 1}`,
url: `/explore/${i + 1}/`,
owners: [],
changed_on_delta_humanized: '1 day ago',
changed_by: null,
dashboards: [],
}));
const { mockOnFetchCharts } = setupTest({
charts: manyCharts.slice(0, 25),
totalCount: 30,
});
// Page 1: Verify UI shows page 1 is active (using Ant Design active class)
await waitFor(() => {
const page1Item = screen.getByRole('listitem', { name: '1' });
expect(page1Item).toHaveClass('ant-pagination-item-active');
});
// Soft assertion: verify component passes correct props to Table
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationCurrent()).toBe(1);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationTotal()).toBe(30);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationPageSize()).toBe(25);
// User clicks page 2 (using userEvent for realistic interaction)
const page2Item = screen.getByRole('listitem', { name: '2' });
await userEvent.click(page2Item);
// Data fetch called for page 2
await waitFor(() =>
expect(mockOnFetchCharts).toHaveBeenCalledWith(
2,
25,
expect.any(String),
expect.any(String),
),
);
// Verify UI reflects page 2 is now active (using Ant Design active class)
await waitFor(() => {
const page2Item = screen.getByRole('listitem', { name: '2' });
expect(page2Item).toHaveClass('ant-pagination-item-active');
});
// Soft assertion: Table still receives correct total (catches the bug!)
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationCurrent()).toBe(2);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationTotal()).toBe(30);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationPageSize()).toBe(25);
});
test('maintains pagination state across multiple page changes', async () => {
const manyCharts = Array.from({ length: 60 }, (_, i) => ({
id: i + 1,
slice_name: `Test Chart ${i + 1}`,
url: `/explore/${i + 1}/`,
owners: [],
changed_on_delta_humanized: '1 day ago',
changed_by: null,
dashboards: [],
}));
const { mockOnFetchCharts } = setupTest({
charts: manyCharts.slice(0, 25),
totalCount: 60,
});
// Verify initial state (page 1) using Ant Design active class
await waitFor(() => {
const page1Item = screen.getByRole('listitem', { name: '1' });
expect(page1Item).toHaveClass('ant-pagination-item-active');
});
// Soft assertion: initial props are correct
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationTotal()).toBe(60);
// User navigates to page 2
await userEvent.click(screen.getByRole('listitem', { name: '2' }));
await waitFor(() => {
expect(mockOnFetchCharts).toHaveBeenCalledWith(
2,
25,
expect.any(String),
expect.any(String),
);
const page2Item = screen.getByRole('listitem', { name: '2' });
expect(page2Item).toHaveClass('ant-pagination-item-active');
});
// Soft assertion: props update correctly after navigation
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationCurrent()).toBe(2);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationTotal()).toBe(60);
// User navigates to page 3
await userEvent.click(screen.getByRole('listitem', { name: '3' }));
await waitFor(() => {
expect(mockOnFetchCharts).toHaveBeenCalledWith(
3,
25,
expect.any(String),
expect.any(String),
);
const page3Item = screen.getByRole('listitem', { name: '3' });
expect(page3Item).toHaveClass('ant-pagination-item-active');
});
// Soft assertion: total remains consistent across all navigations
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationCurrent()).toBe(3);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationTotal()).toBe(60);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationPageSize()).toBe(25);
});
test('handles total count below one page (no pagination needed)', async () => {
const fewCharts = Array.from({ length: 5 }, (_, i) => ({
id: i + 1,
slice_name: `Test Chart ${i + 1}`,
url: `/explore/${i + 1}/`,
owners: [],
changed_on_delta_humanized: '1 day ago',
changed_by: null,
dashboards: [],
}));
setupTest({
charts: fewCharts,
totalCount: 5,
});
// Pagination should not show page 2 item since only 1 page exists
await waitFor(() => {
expect(
screen.queryByRole('listitem', { name: '2' }),
).not.toBeInTheDocument();
});
// Table receives correct props
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationTotal()).toBe(5);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationCurrent()).toBe(1);
});
test('handles server returning fewer rows than page size', async () => {
const partialPage = Array.from({ length: 12 }, (_, i) => ({
id: i + 1,
slice_name: `Test Chart ${i + 1}`,
url: `/explore/${i + 1}/`,
owners: [],
changed_on_delta_humanized: '1 day ago',
changed_by: null,
dashboards: [],
}));
setupTest({
charts: partialPage,
totalCount: 12,
});
// Only page 1 should exist
await waitFor(() => {
expect(
screen.queryByRole('listitem', { name: '2' }),
).not.toBeInTheDocument();
});
// Table receives correct total (not data.length!)
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationTotal()).toBe(12);
// eslint-disable-next-line no-underscore-dangle
expect(tableMock.__getLatestPaginationPageSize()).toBe(25);
});

View File

@@ -99,6 +99,8 @@ const DatasetUsageTab = ({
}: DatasetUsageTabProps) => {
const addDangerToastRef = useRef(addDangerToast);
const tableContainerRef = useRef<HTMLDivElement>(null);
const scrollTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const isMountedRef = useRef(true);
const [loading, setLoading] = useState(false);
const [currentPage, setCurrentPage] = useState(1);
@@ -115,14 +117,19 @@ const DatasetUsageTab = ({
try {
await onFetchCharts(page, PAGE_SIZE, column, direction);
setCurrentPage(page);
setSortColumn(column);
setSortDirection(direction);
// Only update state if component is still mounted
if (isMountedRef.current) {
setCurrentPage(page);
setSortColumn(column);
setSortDirection(direction);
}
} catch (error) {
if (addDangerToastRef.current)
if (isMountedRef.current && addDangerToastRef.current)
addDangerToastRef.current(t('Error fetching charts'));
} finally {
setLoading(false);
if (isMountedRef.current) {
setLoading(false);
}
}
},
[datasourceId, onFetchCharts, sortColumn, sortDirection],
@@ -132,11 +139,31 @@ const DatasetUsageTab = ({
addDangerToastRef.current = addDangerToast;
}, [addDangerToast]);
// Cleanup on unmount
useEffect(() => {
// Set mounted flag to true on mount (important for Strict Mode double-mount)
isMountedRef.current = true;
return () => {
// Mark component as unmounted to prevent state updates
isMountedRef.current = false;
// Clear pending scroll timeout
if (scrollTimeoutRef.current) {
clearTimeout(scrollTimeoutRef.current);
}
};
}, []);
const handlePageChange = useCallback(
(page: number) => {
handleFetchCharts(page);
setTimeout(() => {
// Clear any pending scroll timeout
if (scrollTimeoutRef.current) {
clearTimeout(scrollTimeoutRef.current);
}
scrollTimeoutRef.current = setTimeout(() => {
const tableBody =
tableContainerRef.current?.querySelector('.ant-table-body');
if (tableBody) {
@@ -145,6 +172,7 @@ const DatasetUsageTab = ({
behavior: 'smooth',
});
}
scrollTimeoutRef.current = null;
}, 100);
},
[handleFetchCharts],
@@ -261,14 +289,9 @@ const DatasetUsageTab = ({
sticky
columns={columns}
data={charts}
pagination={{
current: currentPage,
total: totalCount,
pageSize: PAGE_SIZE,
onChange: handlePageChange,
showSizeChanger: false,
size: 'default',
}}
recordCount={totalCount}
usePagination
defaultPageSize={PAGE_SIZE}
loading={loading}
size={TableSize.Middle}
rowKey={(record: Chart) =>
@@ -276,6 +299,12 @@ const DatasetUsageTab = ({
}
tableLayout="fixed"
scroll={{ y: 293, x: '100%' }}
pagination={{
current: currentPage,
total: totalCount,
pageSize: PAGE_SIZE,
hideOnSinglePage: false,
}}
css={css`
.ant-table-pagination.ant-pagination {
margin-bottom: 0;