diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index ac43a67846e..4d53e7ae5f0 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -139,7 +139,7 @@ "react-table": "^7.8.0", "react-transition-group": "^4.4.5", "react-virtualized-auto-sizer": "^1.0.26", - "react-window": "^1.8.10", + "react-window": "^2.3.0", "redux": "^4.2.1", "redux-localstorage": "^0.4.1", "redux-thunk": "^2.1.0", @@ -207,7 +207,6 @@ "@types/react-redux": "^7.1.10", "@types/react-router-dom": "^5.3.3", "@types/react-transition-group": "^4.4.12", - "@types/react-window": "^1.8.8", "@types/redux-localstorage": "^1.0.8", "@types/redux-mock-store": "^1.0.6", "@types/rison": "0.1.0", @@ -12856,15 +12855,6 @@ "@types/react": "*" } }, - "node_modules/@types/react-window": { - "version": "1.8.8", - "resolved": "https://registry.npmjs.org/@types/react-window/-/react-window-1.8.8.tgz", - "integrity": "sha512-8Ls660bHR1AUA2kuRvVG9D/4XpRC6wjAaPT9dil7Ckc76eP9TKWZwwmgfq8Q1LANX3QNDnoU4Zp48A3w+zK69Q==", - "license": "MIT", - "dependencies": { - "@types/react": "*" - } - }, "node_modules/@types/redux-localstorage": { "version": "1.0.12", "resolved": "https://registry.npmjs.org/@types/redux-localstorage/-/redux-localstorage-1.0.12.tgz", @@ -34473,6 +34463,12 @@ "@babel/runtime": "^7.9.2" } }, + "node_modules/react-arborist/node_modules/memoize-one": { + "version": "5.2.1", + "resolved": "https://registry.npmjs.org/memoize-one/-/memoize-one-5.2.1.tgz", + "integrity": "sha512-zYiwtZUcYyXKo/np96AGZAckk+FWWsUdJ3cHGGmld7+AhvcWmQyGCYUh1hc4Q/pkOhb65dQR/pqCyK0cOaHz4Q==", + "license": "MIT" + }, "node_modules/react-arborist/node_modules/react-dnd": { "version": "14.0.5", "resolved": "https://registry.npmjs.org/react-dnd/-/react-dnd-14.0.5.tgz", @@ -34512,6 +34508,23 @@ "dnd-core": "14.0.1" } }, + "node_modules/react-arborist/node_modules/react-window": { + "version": "1.8.11", + "resolved": "https://registry.npmjs.org/react-window/-/react-window-1.8.11.tgz", + "integrity": "sha512-+SRbUVT2scadgFSWx+R1P754xHPEqvcfSfVX10QYg6POOz+WNgkN48pS+BtZNIMGiL1HYrSEiCkwsMS15QogEQ==", + "license": "MIT", + "dependencies": { + "@babel/runtime": "^7.0.0", + "memoize-one": ">=3.1.1 <6" + }, + "engines": { + "node": ">8.0.0" + }, + "peerDependencies": { + "react": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", + "react-dom": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" + } + }, "node_modules/react-arborist/node_modules/redux": { "version": "5.0.1", "resolved": "https://registry.npmjs.org/redux/-/redux-5.0.1.tgz", @@ -35200,28 +35213,15 @@ } }, "node_modules/react-window": { - "version": "1.8.11", - "resolved": "https://registry.npmjs.org/react-window/-/react-window-1.8.11.tgz", - "integrity": "sha512-+SRbUVT2scadgFSWx+R1P754xHPEqvcfSfVX10QYg6POOz+WNgkN48pS+BtZNIMGiL1HYrSEiCkwsMS15QogEQ==", + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/react-window/-/react-window-2.3.0.tgz", + "integrity": "sha512-FW6TIpaOH646k51X7yE+LSCWGkt5Pfsnc1fVyq/sCI9h0pTqmMiBXM04pzFKg3Bt7NGkeV6kqbU8d/QjmFS7Ug==", "license": "MIT", - "dependencies": { - "@babel/runtime": "^7.0.0", - "memoize-one": ">=3.1.1 <6" - }, - "engines": { - "node": ">8.0.0" - }, "peerDependencies": { - "react": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0 || ^19.0.0", - "react-dom": "^15.0.0 || ^16.0.0 || ^17.0.0 || ^18.0.0 || ^19.0.0" + "react": "^18.0.0 || ^19.0.0", + "react-dom": "^18.0.0 || ^19.0.0" } }, - "node_modules/react-window/node_modules/memoize-one": { - "version": "5.2.1", - "resolved": "https://registry.npmjs.org/memoize-one/-/memoize-one-5.2.1.tgz", - "integrity": "sha512-zYiwtZUcYyXKo/np96AGZAckk+FWWsUdJ3cHGGmld7+AhvcWmQyGCYUh1hc4Q/pkOhb65dQR/pqCyK0cOaHz4Q==", - "license": "MIT" - }, "node_modules/read": { "version": "4.1.0", "resolved": "https://registry.npmjs.org/read/-/read-4.1.0.tgz", @@ -42790,7 +42790,6 @@ "@types/lodash": "^4.17.24", "@types/react": "*", "@types/react-loadable": "*", - "@types/react-window": "^1.8.8", "@types/tinycolor2": "*", "typescript": "^5.0.0" }, @@ -42916,7 +42915,6 @@ "@testing-library/user-event": "*", "@types/react": "*", "@types/react-loadable": "*", - "@types/react-window": "^1.8.8", "@types/tinycolor2": "*", "antd": "^6.0.0", "nanoid": "*", @@ -42972,15 +42970,6 @@ "node": ">=12" } }, - "packages/superset-ui-core/node_modules/dompurify": { - "version": "3.4.13", - "resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.13.tgz", - "integrity": "sha512-2vmYIoqjze2d+kakP8S/nS5shfsl587kzwEjcGlTdiksUVgFHnFCsLYDVj/JNqJVOQZGSYBTmuycv0PodwmnMQ==", - "license": "(MPL-2.0 OR Apache-2.0)", - "optionalDependencies": { - "@types/trusted-types": "^2.0.7" - } - }, "packages/superset-ui-core/node_modules/react-ace": { "version": "14.0.1", "resolved": "https://registry.npmjs.org/react-ace/-/react-ace-14.0.1.tgz", diff --git a/superset-frontend/package.json b/superset-frontend/package.json index 4840c664e26..5da9b916e56 100644 --- a/superset-frontend/package.json +++ b/superset-frontend/package.json @@ -216,7 +216,7 @@ "react-table": "^7.8.0", "react-transition-group": "^4.4.5", "react-virtualized-auto-sizer": "^1.0.26", - "react-window": "^1.8.10", + "react-window": "^2.3.0", "redux": "^4.2.1", "redux-localstorage": "^0.4.1", "redux-thunk": "^2.1.0", @@ -284,7 +284,6 @@ "@types/react-redux": "^7.1.10", "@types/react-router-dom": "^5.3.3", "@types/react-transition-group": "^4.4.12", - "@types/react-window": "^1.8.8", "@types/redux-localstorage": "^1.0.8", "@types/redux-mock-store": "^1.0.6", "@types/rison": "0.1.0", diff --git a/superset-frontend/packages/superset-core/package.json b/superset-frontend/packages/superset-core/package.json index 98270cfacf8..e08bd419a54 100644 --- a/superset-frontend/packages/superset-core/package.json +++ b/superset-frontend/packages/superset-core/package.json @@ -109,7 +109,6 @@ "@types/lodash": "^4.17.24", "@types/react": "*", "@types/react-loadable": "*", - "@types/react-window": "^1.8.8", "@types/tinycolor2": "*", "typescript": "^5.0.0" }, diff --git a/superset-frontend/packages/superset-ui-core/package.json b/superset-frontend/packages/superset-ui-core/package.json index 78a335d02b8..1191a74503c 100644 --- a/superset-frontend/packages/superset-ui-core/package.json +++ b/superset-frontend/packages/superset-ui-core/package.json @@ -123,7 +123,6 @@ "@testing-library/user-event": "*", "@types/react": "*", "@types/react-loadable": "*", - "@types/react-window": "^1.8.8", "@types/tinycolor2": "*", "antd": "^6.0.0", "nanoid": "*", diff --git a/superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.test.tsx b/superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.test.tsx new file mode 100644 index 00000000000..f78c55a9381 --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.test.tsx @@ -0,0 +1,122 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render, screen, waitFor } from '@superset-ui/core/spec'; +import type { ColumnsType } from 'antd/es/table'; +import { Table } from './index'; + +// These tests exercise VirtualTable's react-window v2 `Grid` wiring +// (`cellComponent`/`cellProps`/`gridRef`), which previously had no direct +// coverage - `Table.test.tsx` only exercises the non-virtualized code path. + +interface BasicData { + columnName: string; + columnType: string; +} + +const testData: BasicData[] = [ + { columnName: 'Number', columnType: 'Numerical' }, + { columnName: 'String', columnType: 'Physical' }, + { columnName: 'Date', columnType: 'Virtual' }, +]; + +const testColumns: ColumnsType = [ + { + title: 'Column Name', + dataIndex: 'columnName', + key: 'columnName', + width: 150, + }, + { + title: 'Column Type', + dataIndex: 'columnType', + key: 'columnType', + width: 150, + }, +]; + +test('virtualized table renders headers and row content through the react-window Grid', async () => { + render( + , + ); + + await waitFor(() => + testColumns.forEach(column => + expect( + screen + .getAllByText(column.title as string) + .find(el => el.closest('th')), + ).toBeInTheDocument(), + ), + ); + + testData.forEach(row => { + expect(screen.getByText(row.columnName)).toBeInTheDocument(); + }); +}); + +test('virtualized table cells keep the DOM hooks other code (cypress, downloadAsImage) relies on', async () => { + const { container } = render( +
, + ); + + await waitFor(() => { + expect(container.querySelector('.virtual-grid')).toBeInTheDocument(); + expect( + container.querySelectorAll('.virtual-table-cell').length, + ).toBeGreaterThan(0); + }); +}); + +test('cell render functions receive their row data via cellProps rather than a stale closure', async () => { + const columnsWithRender: ColumnsType = [ + { + title: 'Column Name', + dataIndex: 'columnName', + key: 'columnName', + width: 150, + render: (value: string) => `rendered:${value}`, + }, + ]; + + render( +
, + ); + + await waitFor(() => { + expect(screen.getByText('rendered:Number')).toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx b/superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx index 1d5ada1948b..53ef9f30e0a 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx @@ -24,10 +24,14 @@ import { } from 'antd/es/table'; import classNames from 'classnames'; import { useResizeDetector } from 'react-resize-detector'; -import { useEffect, useRef, useState, useCallback, CSSProperties } from 'react'; -import { VariableSizeGrid as Grid } from 'react-window'; +import { useRef, useState, useCallback, type UIEvent } from 'react'; +import { + Grid, + type CellComponentProps, + type GridImperativeAPI, +} from 'react-window'; import { safeHtmlSpan } from '@superset-ui/core'; -import { useTheme, styled } from '@apache-superset/core/theme'; +import { useTheme, styled, SupersetTheme } from '@apache-superset/core/theme'; import { TableSize, ETableAction } from './index'; @@ -71,6 +75,59 @@ const StyledTable = styled(AntTable)( const SMALL = 39; const MIDDLE = 47; +interface VirtualGridCellProps { + mergedColumns: AntTableProps['columns']; + rawData: readonly object[]; + cellSize: number; + allowHTML: boolean; + theme: SupersetTheme; +} + +// Rendered via `cellComponent`, so it must be a stable reference (module scope) +// rather than defined inline on every render of the enclosing table - +// otherwise react-window would treat it as a new component type each render +// and remount every cell. All the data it needs is threaded through +// `cellProps` instead of being closed over. +const VirtualGridCell = ({ + columnIndex, + rowIndex, + style, + mergedColumns, + rawData, + cellSize, + allowHTML, + theme, +}: CellComponentProps) => { + const data: any = rawData?.[rowIndex]; + // Set default content + let content = data?.[(mergedColumns as any)?.[columnIndex]?.dataIndex]; + // Check if the column has a render function + const render = mergedColumns?.[columnIndex]?.render; + if (typeof render === 'function') { + // Use render function to generate formatted content using column's render function + content = render(content, data, rowIndex); + } + + if (allowHTML && typeof content === 'string') { + content = safeHtmlSpan(content); + } + + return ( + + {content} + + ); +}; + const VirtualTable = ( props: VirtualTableProps, ) => { @@ -126,19 +183,15 @@ const VirtualTable = ( (lastColumn.width as number) + Math.floor(tableWidth - totalWidth); } - const gridRef = useRef(); + const gridRef = useRef(null); const [connectObject] = useState(() => { const obj = {}; Object.defineProperty(obj, 'scrollLeft', { - get: () => { - if (gridRef.current) { - return gridRef.current?.state?.scrollLeft; - } - return 0; - }, + get: () => gridRef.current?.element?.scrollLeft ?? 0, set: (scrollLeft: number) => { - if (gridRef.current) { - gridRef.current.scrollTo({ scrollLeft }); + const element = gridRef.current?.element; + if (element) { + element.scrollLeft = scrollLeft; } }, }); @@ -146,14 +199,11 @@ const VirtualTable = ( return obj; }); - const resetVirtualGrid = () => { - gridRef.current?.resetAfterIndices({ - columnIndex: 0, - shouldForceUpdate: true, - }); - }; - - useEffect(() => resetVirtualGrid, [tableWidth, columns, size]); + // No manual cache-reset is needed here (react-window v2 has no + // `resetAfterIndices`-style API): `columnWidth` below is a fresh inline + // closure over `mergedColumns` on every render, so react-window's internal + // size cache - which is invalidated whenever the `columnWidth`/`rowHeight` + // function reference changes - recomputes automatically. /* * antd Table has a runtime error when it tries to fire the onChange event triggered from a pageChange @@ -166,7 +216,10 @@ const VirtualTable = ( * We intentionally leave horizontal scroll where it was so user can focus on * specific range of columns as they page through data */ - gridRef.current?.scrollTo?.({ scrollTop: 0 }); + const element = gridRef.current?.element; + if (element) { + element.scrollTop = 0; + } onChange?.( { @@ -192,61 +245,31 @@ const VirtualTable = ( const cellSize = size === TableSize.Middle ? MIDDLE : SMALL; return ( { const { width = DEFAULT_COL_WIDTH } = mergedColumns[index]; return width as number; }} - height={height || (scroll!.y as number)} rowCount={rawData.length} rowHeight={() => cellSize} - width={tableWidth} - onScroll={({ scrollLeft }: { scrollLeft: number }) => { - onScroll({ scrollLeft }); + style={{ + height: height || (scroll!.y as number), + width: tableWidth, }} - > - {({ - columnIndex, - rowIndex, - style, - }: { - columnIndex: number; - rowIndex: number; - style: CSSProperties; - }) => { - const data: any = rawData?.[rowIndex]; - // Set default content - let content = - data?.[(mergedColumns as any)?.[columnIndex]?.dataIndex]; - // Check if the column has a render function - const render = mergedColumns[columnIndex]?.render; - if (typeof render === 'function') { - // Use render function to generate formatted content using column's render function - content = render(content, data, rowIndex); - } - - if (allowHTML && typeof content === 'string') { - content = safeHtmlSpan(content); - } - - return ( - - {content} - - ); + cellComponent={VirtualGridCell} + cellProps={{ + mergedColumns, + rawData, + cellSize, + allowHTML, + theme, }} - + onScroll={(event: UIEvent) => { + onScroll({ scrollLeft: event.currentTarget.scrollLeft }); + }} + /> ); }; diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx index 9001656f02a..f031a28c5f1 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx @@ -18,7 +18,6 @@ */ import { - CSSProperties, ReactNode, useCallback, useEffect, @@ -44,7 +43,7 @@ import { Icons, } from '@superset-ui/core/components'; import { debounce } from 'lodash-es'; -import { FixedSizeList as List } from 'react-window'; +import { List, type RowComponentProps } from 'react-window'; import { InputRef } from 'antd'; import { MenuItemTooltip } from '../DisabledMenuItemTooltip'; import { VirtualizedMenuItem } from '../MenuItemWithTruncation'; @@ -53,6 +52,34 @@ import { Dataset } from '../types'; const SUBMENU_HEIGHT = 200; const SHOW_COLUMNS_SEARCH_THRESHOLD = 10; +interface DrillByColumnRowProps { + columns: Column[]; + onSelectColumn: (event: React.MouseEvent, column: Column) => void; +} + +// Rendered via `rowComponent`, so it must be a stable reference (module +// scope) rather than defined inline on every render of the submenu - +// otherwise react-window would treat it as a new component type each +// render and remount every row. All the data it needs is threaded +// through `rowProps` instead of being closed over. +function DrillByColumnRow({ + index, + style, + columns, + onSelectColumn, +}: RowComponentProps) { + const column = columns[index]; + return ( + onSelectColumn(e, column)} + style={style} + > + {column.verbose_name || column.column_name} + + ); +} + export interface DrillBySubmenuProps { drillByConfig?: ContextMenuFilters['drillBy']; formData: BaseFormData & { [key: string]: any }; @@ -190,26 +217,9 @@ export const DrillBySubmenu = ({ const isDisabled = !handlesDimensionContextMenu || !hasDrillBy; - const Row = ({ - index, - data, - style, - }: { - index: number; - data: { columns: Column[] }; - style: CSSProperties; - }) => { - const { columns } = data; - const column = columns[index]; - return ( - handleSelection(e, column)} - style={style} - > - {column.verbose_name || column.column_name} - - ); + const listRowProps: DrillByColumnRowProps = { + columns: filteredColumns, + onSelectColumn: handleSelection, }; const popoverContent = ( @@ -260,15 +270,13 @@ export const DrillBySubmenu = ({ ) : filteredColumns.length ? ( - {Row} - + /> ) : (
({ - VariableSizeList: ({ - children: Row, - itemCount, - itemData, + List: ({ + rowComponent: Row, + rowCount, + rowProps, }: { - children: React.ComponentType<{ - index: number; - style: React.CSSProperties; - data: unknown; - }>; - itemCount: number; - itemData: unknown; + rowComponent: React.ComponentType< + { index: number; style: React.CSSProperties } & Record + >; + rowCount: number; + rowProps: Record; }) => (
- {Array.from({ length: itemCount }, (_, index) => ( + {Array.from({ length: rowCount }, (_, index) => ( ))}
diff --git a/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeItem.tsx b/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeItem.tsx index 9bfda10c8a3..bd0a373fb04 100644 --- a/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeItem.tsx +++ b/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeItem.tsx @@ -18,7 +18,7 @@ */ import { CSSProperties, memo } from 'react'; -import type { ListChildComponentProps } from 'react-window'; +import type { RowComponentProps } from 'react-window'; import { useDroppable } from '@dnd-kit/core'; import type { UniqueIdentifier } from '@dnd-kit/core'; import type { Metric, ColumnMeta } from '@superset-ui/chart-controls'; @@ -135,32 +135,34 @@ const TreeItemWrapper = memo(function TreeItemWrapper({ ); }); -function VirtualizedTreeItemComponent({ +// Not wrapped in `memo()` here: react-window v2 already wraps `rowComponent` +// in its own `memo()` internally (comparing `ariaAttributes`/`style`/the +// rest of the row props), so an outer `memo()` would be redundant - and +// React 19's `memo()` typings widen the wrapped component's return type to +// `ReactNode`, which react-window v2's `rowComponent` type (expecting +// `ReactElement | null`) rejects. +export function VirtualizedTreeItem({ index, style, - data, -}: ListChildComponentProps) { - const { - flattenedItems, - collapsedIds, - selectedItemIds, - editingFolderId, - folderChildCounts, - itemSeparatorInfo, - visibleItemIds, - searchTerm, - metricsMap, - columnsMap, - activeId, - draggedFolderChildIds, - forbiddenDropFolderIds, - currentDropTargetId, - onToggleCollapse, - onSelect, - onStartEdit, - onFinishEdit, - } = data; - + flattenedItems, + collapsedIds, + selectedItemIds, + editingFolderId, + folderChildCounts, + itemSeparatorInfo, + visibleItemIds, + searchTerm, + metricsMap, + columnsMap, + activeId, + draggedFolderChildIds, + forbiddenDropFolderIds, + currentDropTargetId, + onToggleCollapse, + onSelect, + onStartEdit, + onFinishEdit, +}: RowComponentProps) { const item = flattenedItems[index]; if (!item) { @@ -224,5 +226,3 @@ function VirtualizedTreeItemComponent({ /> ); } - -export const VirtualizedTreeItem = memo(VirtualizedTreeItemComponent); diff --git a/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeList.tsx b/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeList.tsx index eeb2dad76b5..0f27036b0fb 100644 --- a/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeList.tsx +++ b/superset-frontend/src/components/Datasource/FoldersEditor/VirtualizedTreeList.tsx @@ -17,8 +17,8 @@ * under the License. */ -import { useCallback, useEffect, useMemo, useRef } from 'react'; -import { VariableSizeList as List } from 'react-window'; +import { useCallback, useMemo, useRef } from 'react'; +import { List, type ListImperativeAPI } from 'react-window'; import type { UniqueIdentifier } from '@dnd-kit/core'; import type { Metric, ColumnMeta } from '@superset-ui/chart-controls'; import { FoldersEditorItemType } from '../types'; @@ -82,7 +82,7 @@ export function VirtualizedTreeList({ onStartEdit, onFinishEdit, }: VirtualizedTreeListProps) { - const listRef = useRef(null); + const listRef = useRef(null); const containerRef = useRef(null); // Custom auto-scroll during drag (replaces dnd-kit's auto-scroll which conflicts with virtualization) @@ -93,20 +93,15 @@ export function VirtualizedTreeList({ listHeight: height, }); - // Reset list cache when items structure changes, but not during drag - // Resetting during drag causes jumping/flickering - useEffect(() => { - if (!isDragging) { - listRef.current?.resetAfterIndex(0); - } - }, [ - flattenedItems, - collapsedIds, - folderChildCounts, - itemSeparatorInfo, - visibleItemIds, - isDragging, - ]); + // No manual cache-reset is needed here (react-window v2 has no + // `resetAfterIndex`-style API): `getItemSize` below is a `useCallback` + // whose dependency list matches everything that can change row heights, + // so react-window's internal size cache - which is invalidated whenever + // the `rowHeight` function reference changes - recomputes automatically. + // Because `flattenedItems` is held stable during drag (see + // useDragHandlers), `getItemSize`'s reference - and therefore the cache - + // naturally stays stable during drag too, preserving the old "don't + // reset while dragging" behavior without needing an explicit gate. // Calculate item size for react-window const getItemSize = useCallback( @@ -169,7 +164,7 @@ export function VirtualizedTreeList({ ); // Prepare item data for the row renderer - const itemData: VirtualizedTreeItemData = useMemo( + const rowProps: VirtualizedTreeItemData = useMemo( () => ({ flattenedItems, collapsedIds, @@ -218,16 +213,14 @@ export function VirtualizedTreeList({ return (
- {VirtualizedTreeItem} - + />
); } diff --git a/superset-frontend/src/components/Datasource/FoldersEditor/hooks/useAutoScroll.ts b/superset-frontend/src/components/Datasource/FoldersEditor/hooks/useAutoScroll.ts index a9da33039d5..1e30affd771 100644 --- a/superset-frontend/src/components/Datasource/FoldersEditor/hooks/useAutoScroll.ts +++ b/superset-frontend/src/components/Datasource/FoldersEditor/hooks/useAutoScroll.ts @@ -18,7 +18,7 @@ */ import { useEffect, useRef } from 'react'; -import type { VariableSizeList as List } from 'react-window'; +import type { ListImperativeAPI } from 'react-window'; // Distance from edge where auto-scroll activates (in pixels) const SCROLL_THRESHOLD = 80; @@ -28,7 +28,7 @@ const BASE_SCROLL_SPEED = 8; const MAX_SPEED_MULTIPLIER = 3; interface UseAutoScrollOptions { - listRef: React.RefObject; + listRef: React.RefObject; containerRef: React.RefObject; isDragging: boolean; listHeight: number; @@ -110,7 +110,7 @@ export function useAutoScroll({ // Animation frame callback - uses time-based scrolling for consistent speed const scrollFrame = (currentTime: number) => { const list = listRef.current; - const outerElement = (list as any)?._outerRef; + const outerElement = list?.element; if (!list || !outerElement || !state.direction) { // Restore pointer events when scrolling stops @@ -149,7 +149,7 @@ export function useAutoScroll({ if (Math.abs(newScroll - currentScroll) > 0.5) { // Use direct DOM manipulation for smoother scrolling - // react-window's scrollTo triggers re-renders which can cause stutter + // react-window's scrollToRow triggers re-renders which can cause stutter outerElement.scrollTop = newScroll; } diff --git a/superset-frontend/src/dashboard/components/SliceAdder.test.tsx b/superset-frontend/src/dashboard/components/SliceAdder.test.tsx index 9cdac1de085..9b34aaa769b 100644 --- a/superset-frontend/src/dashboard/components/SliceAdder.test.tsx +++ b/superset-frontend/src/dashboard/components/SliceAdder.test.tsx @@ -25,7 +25,11 @@ import { import { DatasourceType } from '@superset-ui/core'; import { sliceEntitiesForDashboard as mockSliceEntities } from 'spec/fixtures/mockSliceEntities'; import { configureStore } from '@reduxjs/toolkit'; -import SliceAdder, { SliceAdderProps, sortByComparator } from './SliceAdder'; +import SliceAdder, { + SliceAdderProps, + sortByComparator, + SliceListRow, +} from './SliceAdder'; // Mock the Select component to avoid debounce issues jest.mock('@superset-ui/core', () => ({ @@ -236,4 +240,54 @@ describe('SliceAdder', () => { // Verify the internal state was updated by checking if new charts are available expect(screen.getByRole('checkbox')).toBeInTheDocument(); }); + + // Covers the react-window v2 `rowComponent`/`rowProps` wiring: SliceAdder + // hoists its row renderer to module scope (see SliceAdder.tsx) so + // react-window doesn't remount every row on each render. This verifies + // that hoisted row renders correctly from the flattened rowProps react-window + // passes it (index/style plus the custom row props), rather than through + // closures over component state. + test('SliceListRow renders a chart card for the slice at the given index', () => { + const filteredSlices = Object.values(mockSliceEntities.slices); + render( + , + { useDnd: true, store: mockStore }, + ); + expect(screen.getByTestId('chart-card')).toBeInTheDocument(); + expect(screen.getByText(filteredSlices[0].slice_name)).toBeInTheDocument(); + }); + + test('SliceListRow renders the slice at the row index passed via rowProps, not index 0', () => { + const filteredSlices = Object.values(mockSliceEntities.slices); + render( + , + { useDnd: true, store: mockStore }, + ); + expect(screen.getByText(filteredSlices[1].slice_name)).toBeInTheDocument(); + expect( + screen.queryByText(filteredSlices[0].slice_name), + ).not.toBeInTheDocument(); + }); }); diff --git a/superset-frontend/src/dashboard/components/SliceAdder.tsx b/superset-frontend/src/dashboard/components/SliceAdder.tsx index 5abc643ed8f..f260bc0a2d5 100644 --- a/superset-frontend/src/dashboard/components/SliceAdder.tsx +++ b/superset-frontend/src/dashboard/components/SliceAdder.tsx @@ -19,7 +19,7 @@ /* eslint-env browser */ import { useState, useEffect, useCallback, useRef, useMemo } from 'react'; import AutoSizer from 'react-virtualized-auto-sizer'; -import { FixedSizeList as List } from 'react-window'; +import { List, type RowComponentProps } from 'react-window'; // @ts-expect-error import { createFilter } from 'react-search-input'; import { t } from '@apache-superset/core/translation'; @@ -167,6 +167,71 @@ function getFilteredSortedSlices( .sort(sortByComparator(sortBy)); } +export interface SliceListRowProps { + filteredSlices: Slice[]; + selectedSliceIdsSet: Set; + editMode: boolean; +} + +// Rendered via `rowComponent`, so it must be a stable reference (module +// scope) rather than defined inline on every render of SliceAdder - +// otherwise react-window would treat it as a new component type each +// render and remount every row (losing in-flight drag state). All the +// data it needs is threaded through `rowProps` instead of being closed +// over. +export function SliceListRow({ + index, + style, + filteredSlices, + selectedSliceIdsSet, + editMode, +}: RowComponentProps) { + const cellData = filteredSlices[index]; + + const isSelected = selectedSliceIdsSet.has(cellData.slice_id); + const type = CHART_TYPE; + const id = NEW_CHART_ID; + + const meta = { + chartId: cellData.slice_id, + sliceName: cellData.slice_name, + }; + return ( + + {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( + + )} + + ); +} + function SliceAdder({ fetchSlices, updateSlices, @@ -321,53 +386,8 @@ function SliceAdder({ [fetchSlices, searchTerm, sortBy], ); - const rowRenderer = useCallback( - ({ index, style }: { index: number; style: React.CSSProperties }) => { - const cellData = filteredSlices[index]; - - const isSelected = selectedSliceIdsSet.has(cellData.slice_id); - const type = CHART_TYPE; - const id = NEW_CHART_ID; - - const meta = { - chartId: cellData.slice_id, - sliceName: cellData.slice_name, - }; - return ( - - {({ dragSourceRef }: { dragSourceRef: ConnectDragSource }) => ( - - )} - - ); - }, + const listRowProps = useMemo( + () => ({ filteredSlices, selectedSliceIdsSet, editMode }), [filteredSlices, selectedSliceIdsSet, editMode], ); @@ -448,14 +468,13 @@ function SliceAdder({ {({ height, width }: { height: number; width: number }) => ( filteredSlices[index].slice_id} - > - {rowRenderer} - + style={{ width, height }} + rowCount={filteredSlices.length} + rowHeight={DEFAULT_CELL_HEIGHT} + rowProps={listRowProps} + rowComponent={SliceListRow} + rowKey={(index, data) => data.filteredSlices[index].slice_id} + /> )} diff --git a/superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx b/superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx index 53e55b60543..8da7586f9c0 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx @@ -16,8 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; -import { VariableSizeList as List } from 'react-window'; +import { useCallback, useMemo, useState } from 'react'; +import { List } from 'react-window'; import { FlattenedItem, Folder } from './types'; import DatasourcePanelItem from './DatasourcePanelItem'; @@ -98,7 +98,6 @@ export const DatasourceItems = ({ height, folders, }: DatasourceItemsProps) => { - const listRef = useRef(null); const [collapsedFolderIds, setCollapsedFolderIds] = useState>( new Set( folders.filter(folder => folder.isCollapsed).map(folder => folder.id), @@ -122,17 +121,15 @@ export const DatasourceItems = ({ }); }, []); - useEffect(() => { - // reset the list cache when flattenedItems length changes to recalculate the heights - listRef.current?.resetAfterIndex(0); - }, [flattenedItems]); - - const getItemSize = useCallback( + // No manual cache-reset is needed: `rowHeight` is a useCallback tied to + // `flattenedItems`, so react-window's internal size cache recomputes + // automatically whenever its reference changes. + const rowHeight = useCallback( (index: number) => flattenedItems[index].height, [flattenedItems], ); - const itemData = useMemo( + const rowProps = useMemo( () => ({ flattenedItems, folderMap, @@ -151,15 +148,12 @@ export const DatasourceItems = ({ return ( - {DatasourcePanelItem} - + /> ); }; diff --git a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.test.tsx b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.test.tsx index 37449902b5e..f3859a9f768 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.test.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.test.tsx @@ -22,12 +22,12 @@ import { } from 'src/explore/components/DatasourcePanel/fixtures'; import { screen, userEvent, render } from 'spec/helpers/testing-library'; import DatasourcePanelItem, { - DatasourcePanelItemProps, + DatasourcePanelItemRowProps, } from './DatasourcePanelItem'; import { FoldersEditorItemType } from 'src/components/Datasource/types'; import { MetricItem, ColumnItem } from './types'; -const mockData: DatasourcePanelItemProps['data'] = { +const mockData: DatasourcePanelItemRowProps = { flattenedItems: [ { type: 'header', depth: 0, folderId: '1', height: 50 }, ...metrics.map((m, idx) => ({ @@ -82,11 +82,22 @@ const mockData: DatasourcePanelItemProps['data'] = { collapsedFolderIds: new Set(), }; -const setup = (data: DatasourcePanelItemProps['data'] = mockData) => +const setup = (data: DatasourcePanelItemRowProps = mockData) => render( <> {data.flattenedItems.map((_, index) => ( - + ))} , { useDnd: true, useRedux: true, initialState: { explore: {} } }, diff --git a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.tsx b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.tsx index c19753eaa3d..fed70cdab93 100644 --- a/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.tsx +++ b/superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.tsx @@ -16,7 +16,8 @@ * specific language governing permissions and limitations * under the License. */ -import { CSSProperties, ReactNode, useCallback } from 'react'; +import { ReactNode, useCallback } from 'react'; +import type { RowComponentProps } from 'react-window'; import { t } from '@apache-superset/core/translation'; import { useCSSTextTruncation } from '@superset-ui/core'; @@ -112,30 +113,26 @@ const Divider = styled.div` `} `; -export interface DatasourcePanelItemProps { - index: number; - style: CSSProperties; - data: { - flattenedItems: FlattenedItem[]; - folderMap: Map; - width: number; - onToggleCollapse: (folderId: string) => void; - collapsedFolderIds: Set; - }; +export interface DatasourcePanelItemRowProps { + flattenedItems: FlattenedItem[]; + folderMap: Map; + width: number; + onToggleCollapse: (folderId: string) => void; + collapsedFolderIds: Set; } +export type DatasourcePanelItemProps = + RowComponentProps; + const DatasourcePanelItem = ({ index, style, - data, + flattenedItems, + folderMap, + width, + onToggleCollapse, + collapsedFolderIds, }: DatasourcePanelItemProps) => { - const { - flattenedItems, - folderMap, - width, - onToggleCollapse, - collapsedFolderIds, - } = data; const item = flattenedItems[index]; const theme = useTheme(); const [labelRef, labelIsTruncated] = useCSSTextTruncation({