Copilot commented on code in PR #36345:
URL: https://github.com/apache/superset/pull/36345#discussion_r2578275024
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -284,36 +291,80 @@ export function ListView<T extends object = any>({
addSuccessToast,
addDangerToast,
}: ListViewProps<T>) {
+ // Pro Table action ref for programmatic control
+ const actionRef = useRef<ActionType>();
+
+ // Use the simplified Pro Table state hook (no react-table dependency)
const {
- getTableProps,
- getTableBodyProps,
- headerGroups,
- rows,
- prepareRow,
- pageCount = 1,
+ pageIndex,
+ pageSize,
+ pageCount,
gotoPage,
- applyFilterValue,
+ sortBy,
setSortBy,
- selectedFlatRows,
- toggleAllRowsSelected,
+ internalFilters,
+ applyFilterValue,
+ viewMode,
setViewMode,
- state: { pageIndex, pageSize, internalFilters, sortBy, viewMode },
+ selectedRowKeys,
+ setSelectedRowKeys,
+ selectedRows,
+ toggleAllRowsSelected,
query,
- } = useListViewState({
- bulkSelectColumnConfig,
- bulkSelectMode: bulkSelectEnabled && Boolean(bulkActions.length),
- columns,
- count,
- data,
+ } = useProTableState({
fetchData,
+ data,
+ count,
initialPageSize,
initialSort,
initialFilters: filters,
renderCard: Boolean(renderCard),
defaultViewMode,
});
+
const allowBulkTagActions = bulkTagResourceName && enableBulkTag;
const filterable = Boolean(filters.length);
+
+ // Convert columns directly to ProColumns format (no react-table mapping)
+ const proColumns = useMemo<ProColumns<T & { _rowKey: string }>[]>(() => {
+ const currentSort = sortBy[0];
+ return columns.map(col => {
+ const dataIndex = col.accessor || col.id;
+ return {
+ title: col.Header,
+ dataIndex: dataIndex?.includes('.') ? dataIndex.split('.') : dataIndex,
+ key: col.id,
+ hidden: col.hidden,
+ width: col.size ? COLUMN_SIZE_MAP[col.size] : undefined,
+ ellipsis: !columnsForWrapText?.includes(col.id),
+ sorter: !col.disableSortBy,
+ defaultSortOrder:
+ currentSort?.id === col.id
+ ? currentSort.desc
+ ? 'descend'
+ : 'ascend'
+ : undefined,
+ className: col.className,
+ render: col.Cell
+ ? (_: unknown, record: T & { _rowKey: string }) =>
+ col.Cell({
+ value: dataIndex
+ ? dataIndex.split('.').reduce((obj: any, key: string) =>
obj?.[key], record)
+ : undefined,
+ row: { original: record, id: record._rowKey },
+ })
+ : undefined,
+ };
+ });
+ }, [columns, sortBy, columnsForWrapText]);
+
+ // Add row keys to data for selection and identification
+ const dataWithKeys = useMemo(
+ () => data.map((row, index) => ({ ...row, _rowKey: String(index) })),
Review Comment:
Using array index as row ID will cause issues with selection state. When a
user selects rows on page 1 (indices 0-24) and then navigates to page 2 (which
also has indices 0-24), the selection state from page 1 will incorrectly appear
on page 2's rows.
This is the same underlying issue as in `useProTableState.ts`. Consider
using a unique identifier from the row data (e.g., `row.id`) as the key instead
of the array index.
```suggestion
() =>
data.map((row, index) => {
// Use row.id if available, otherwise throw an error
if (row.id === undefined || row.id === null) {
throw new ListViewError(
'Each row must have a unique "id" property for selection state.
Please ensure your data includes "id".',
);
}
return { ...row, _rowKey: String(row.id) };
}),
```
##########
superset-frontend/package.json:
##########
@@ -93,6 +93,7 @@
"last 3 edge versions"
],
"dependencies": {
+ "@ant-design/pro-components" : "^2.8.10",
Review Comment:
[nitpick] Inconsistent spacing around the colon. The property name has an
extra space before the colon which is inconsistent with the formatting of other
dependencies in the file. Should be `"@ant-design/pro-components": "^2.8.10",`
without the space before the colon.
```suggestion
"@ant-design/pro-components": "^2.8.10",
```
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -284,36 +291,80 @@ export function ListView<T extends object = any>({
addSuccessToast,
addDangerToast,
}: ListViewProps<T>) {
+ // Pro Table action ref for programmatic control
+ const actionRef = useRef<ActionType>();
+
+ // Use the simplified Pro Table state hook (no react-table dependency)
const {
- getTableProps,
- getTableBodyProps,
- headerGroups,
- rows,
- prepareRow,
- pageCount = 1,
+ pageIndex,
+ pageSize,
+ pageCount,
gotoPage,
- applyFilterValue,
+ sortBy,
setSortBy,
- selectedFlatRows,
- toggleAllRowsSelected,
+ internalFilters,
+ applyFilterValue,
+ viewMode,
setViewMode,
- state: { pageIndex, pageSize, internalFilters, sortBy, viewMode },
+ selectedRowKeys,
+ setSelectedRowKeys,
+ selectedRows,
+ toggleAllRowsSelected,
query,
- } = useListViewState({
- bulkSelectColumnConfig,
- bulkSelectMode: bulkSelectEnabled && Boolean(bulkActions.length),
- columns,
- count,
- data,
+ } = useProTableState({
fetchData,
+ data,
+ count,
initialPageSize,
initialSort,
initialFilters: filters,
renderCard: Boolean(renderCard),
defaultViewMode,
});
+
const allowBulkTagActions = bulkTagResourceName && enableBulkTag;
const filterable = Boolean(filters.length);
+
+ // Convert columns directly to ProColumns format (no react-table mapping)
+ const proColumns = useMemo<ProColumns<T & { _rowKey: string }>[]>(() => {
+ const currentSort = sortBy[0];
+ return columns.map(col => {
+ const dataIndex = col.accessor || col.id;
+ return {
+ title: col.Header,
+ dataIndex: dataIndex?.includes('.') ? dataIndex.split('.') : dataIndex,
+ key: col.id,
+ hidden: col.hidden,
+ width: col.size ? COLUMN_SIZE_MAP[col.size] : undefined,
+ ellipsis: !columnsForWrapText?.includes(col.id),
+ sorter: !col.disableSortBy,
+ defaultSortOrder:
+ currentSort?.id === col.id
+ ? currentSort.desc
+ ? 'descend'
+ : 'ascend'
+ : undefined,
+ className: col.className,
+ render: col.Cell
+ ? (_: unknown, record: T & { _rowKey: string }) =>
+ col.Cell({
+ value: dataIndex
+ ? dataIndex.split('.').reduce((obj: any, key: string) =>
obj?.[key], record)
Review Comment:
Potential runtime error when accessing nested properties. If `dataIndex` is
an array (line 335), calling `dataIndex.split('.')` on line 352 will fail since
arrays don't have a `split` method.
To fix this, update line 352 to handle both string and array cases:
```typescript
value: dataIndex
? (Array.isArray(dataIndex) ? dataIndex :
dataIndex.split('.')).reduce((obj: any, key: string) => obj?.[key], record)
: undefined,
```
```suggestion
? (Array.isArray(dataIndex) ? dataIndex :
dataIndex.split('.')).reduce((obj: any, key: string) => obj?.[key], record)
```
##########
superset-frontend/src/components/ListView/useProTableState.ts:
##########
@@ -0,0 +1,336 @@
+/**
+ * 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.
+ */
+
+/**
+ * A simplified state management hook for ProTable that replaces the heavy
+ * react-table based useListViewState. This hook manages pagination, sorting,
+ * filtering, and view mode state while syncing with URL query params.
+ */
+
+import { useEffect, useMemo, useState, useCallback } from 'react';
+import {
+ NumberParam,
+ StringParam,
+ useQueryParams,
+ QueryParamConfig,
+} from 'use-query-params';
+import rison from 'rison';
+import {
+ ListViewFetchDataConfig as FetchDataConfig,
+ ListViewFilter as Filter,
+ ListViewFilterValue as FilterValue,
+ InnerFilterValue,
+ InternalFilter,
+ SortColumn,
+ ViewModeType,
+} from './types';
+
+// Custom RisonParam for proper encoding/decoding
+const RisonParam: QueryParamConfig<string, Record<string, InnerFilterValue>> =
{
+ encode: (data?: Record<string, InnerFilterValue> | null) => {
+ if (data === undefined || data === null) return undefined;
+
+ const cleanData = JSON.parse(
+ JSON.stringify(data, (_, value) => (value === undefined ? null : value)),
+ );
+
+ return rison
+ .encode(cleanData)
+ .replace(/%/g, '%25')
+ .replace(/&/g, '%26')
+ .replace(/\+/g, '%2B')
+ .replace(/#/g, '%23');
+ },
+ decode: (dataStr?: string | string[]) =>
+ dataStr === undefined || Array.isArray(dataStr)
+ ? undefined
+ : rison.decode(dataStr),
+};
+
+export interface UseProTableStateConfig<T extends object> {
+ fetchData: (conf: FetchDataConfig) => void;
+ data: T[];
+ count: number;
+ initialPageSize: number;
+ initialSort?: SortColumn[];
+ initialFilters?: Filter[];
+ bulkSelectMode?: boolean;
+ renderCard?: boolean;
+ defaultViewMode?: ViewModeType;
+}
+
+export interface ProTableState<T extends object> {
+ // Pagination
+ pageIndex: number;
+ pageSize: number;
+ pageCount: number;
+ gotoPage: (page: number) => void;
+
+ // Sorting
+ sortBy: SortColumn[];
+ setSortBy: (sortBy: SortColumn[]) => void;
+
+ // Filtering
+ internalFilters: InternalFilter[];
+ applyFilterValue: (index: number, value: InnerFilterValue) => void;
+
+ // View mode
+ viewMode: ViewModeType;
+ setViewMode: (mode: ViewModeType) => void;
+
+ // Selection
+ selectedRowKeys: string[];
+ setSelectedRowKeys: (keys: string[]) => void;
+ selectedRows: T[];
+ toggleAllRowsSelected: (selected: boolean) => void;
+
+ // Query state for empty state detection
+ query: {
+ filters?: Record<string, InnerFilterValue>;
+ pageIndex?: number | null;
+ sortColumn?: string | null;
+ sortOrder?: string | null;
+ viewMode?: string | null;
+ };
+}
+
+function mergeCreateFilterValues(
+ list: Filter[],
+ updateObj: Record<string, InnerFilterValue>,
+): InternalFilter[] {
+ return list.map(({ id, urlDisplay, operator }) => {
+ const currentFilterId = urlDisplay || id;
+ const update = updateObj[currentFilterId];
+ return { id, urlDisplay, operator, value: update };
+ });
+}
+
+function convertFilters(fts: InternalFilter[]): FilterValue[] {
+ return fts
+ .filter(
+ f =>
+ !(
+ typeof f.value === 'undefined' ||
+ (Array.isArray(f.value) && !f.value.length)
+ ),
+ )
+ .map(({ value, operator, id }) => {
+ if (operator === 'between' && Array.isArray(value)) {
+ return [
+ { value: value[0], operator: 'gt', id },
+ { value: value[1], operator: 'lt', id },
+ ];
+ }
+ return { value, operator, id };
+ })
+ .flat();
+}
+
+export function useProTableState<T extends object>({
+ fetchData,
+ data,
+ count,
+ initialPageSize,
+ initialFilters = [],
+ initialSort = [],
+ renderCard = false,
+ defaultViewMode = 'card',
+}: UseProTableStateConfig<T>): ProTableState<T> {
+ // URL query params
+ const [query, setQuery] = useQueryParams({
+ filters: RisonParam,
+ pageIndex: NumberParam,
+ sortColumn: StringParam,
+ sortOrder: StringParam,
+ viewMode: StringParam,
+ });
+
+ // Pagination state
+ const [pageIndex, setPageIndex] = useState<number>(query.pageIndex || 0);
+ const [pageSize] = useState<number>(initialPageSize);
+ const pageCount = useMemo(
+ () => Math.ceil(count / initialPageSize),
+ [count, initialPageSize],
+ );
+
+ // Sorting state
+ const [sortBy, setSortByState] = useState<SortColumn[]>(() => {
+ if (query.sortColumn && query.sortOrder) {
+ return [{ id: query.sortColumn, desc: query.sortOrder === 'desc' }];
+ }
+ return initialSort;
+ });
+
+ // Filter state
+ const [internalFilters, setInternalFilters] = useState<InternalFilter[]>(
+ () =>
+ query.filters && initialFilters.length
+ ? mergeCreateFilterValues(initialFilters, query.filters)
+ : [],
Review Comment:
The filter initialization logic returns an empty array when `query.filters`
exists but `initialFilters` is empty. This could cause a mismatch between URL
state and component state. If there are filters in the URL but no
`initialFilters` provided, those filters will be lost.
Consider initializing with URL filters even when `initialFilters` is empty:
```typescript
const [internalFilters, setInternalFilters] = useState<InternalFilter[]>(
() =>
query.filters && initialFilters.length
? mergeCreateFilterValues(initialFilters, query.filters)
: initialFilters.length
? initialFilters.map(f => ({ ...f, value: undefined }))
: [],
);
```
```suggestion
() => {
if (query.filters && initialFilters.length) {
return mergeCreateFilterValues(initialFilters, query.filters);
}
if (query.filters && !initialFilters.length) {
// If initialFilters is empty, use query.filters directly
// Assuming query.filters is an array of InternalFilter or
compatible shape
return Array.isArray(query.filters)
? query.filters.map(f => ({ ...f }))
: [];
}
if (initialFilters.length) {
return initialFilters.map(f => ({ ...f, value: undefined }));
}
return [];
},
```
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -339,23 +390,95 @@ export function ListView<T extends object = any>({
const cardViewEnabled = Boolean(renderCard);
const [showBulkTagModal, setShowBulkTagModal] = useState<boolean>(false);
+ // Row selection config for Pro Table
+ const rowSelection = useMemo(
+ () =>
+ bulkSelectEnabled
+ ? {
+ selectedRowKeys,
+ onChange: (keys: React.Key[]) => {
+ setSelectedRowKeys(keys as string[]);
+ },
+ }
+ : undefined,
+ [bulkSelectEnabled, selectedRowKeys, setSelectedRowKeys],
+ );
+
+ // Pro Table pagination config
+ const paginationConfig = useMemo(() => {
+ if (count === 0) return false;
+ return {
+ current: pageIndex + 1,
+ pageSize,
+ total: count,
+ showSizeChanger: false,
+ showQuickJumper: false,
+ onChange: (page: number) => {
+ gotoPage(Math.max(0, page - 1));
+ },
+ };
+ }, [count, gotoPage, pageIndex, pageSize]);
+
+ // Handle Pro Table sorting
+ const handleTableChange = useCallback(
+ (_pagination: unknown, _filters: unknown, sorter: unknown) => {
+ const normalizedSorter = Array.isArray(sorter) ? sorter[0] : sorter;
+ const sorterObj = normalizedSorter as
+ | { field?: string | string[] | number; order?: string }
+ | undefined;
+ if (sorterObj?.field) {
+ const fieldId = Array.isArray(sorterObj.field)
+ ? sorterObj.field.join('.')
+ : String(sorterObj.field);
+ setSortBy([
+ {
+ id: fieldId,
+ desc: sorterObj.order === 'descend',
+ },
+ ]);
+ }
+ },
+ [setSortBy],
+ );
+
+ // Clear selections when bulk select is disabled
useEffect(() => {
- // discard selections if bulk select is disabled
- if (!bulkSelectEnabled) toggleAllRowsSelected(false);
- }, [bulkSelectEnabled, toggleAllRowsSelected]);
+ if (!bulkSelectEnabled) {
+ setSelectedRowKeys([]);
+ }
+ }, [bulkSelectEnabled, setSelectedRowKeys]);
+ // Reset to first page if current page is invalid
useEffect(() => {
if (!loading && pageIndex > pageCount - 1 && pageCount > 0) {
gotoPage(0);
}
}, [gotoPage, loading, pageCount, pageIndex]);
+ // For CardCollection compatibility - create row objects with selection
methods
+ const cardRows = useMemo(
+ () =>
+ dataWithKeys.map((row, index) => ({
+ id: String(index),
+ original: row,
+ isSelected: selectedRowKeys.includes(String(index)),
+ toggleRowSelected: (value?: boolean) => {
+ const newKeys =
+ value ?? !selectedRowKeys.includes(String(index))
+ ? [...selectedRowKeys, String(index)]
+ : selectedRowKeys.filter(k => k !== String(index));
Review Comment:
Logic error in `toggleRowSelected`. The ternary operator evaluates
incorrectly. When `value` is `false`, the expression `value ??
!selectedRowKeys.includes(String(index))` will evaluate to `false` (not using
the fallback), but the condition should check if the row should be selected or
deselected.
The correct logic should be:
```typescript
toggleRowSelected: (value?: boolean) => {
const shouldSelect = value !== undefined ? value :
!selectedRowKeys.includes(String(index));
const newKeys = shouldSelect
? [...selectedRowKeys, String(index)]
: selectedRowKeys.filter(k => k !== String(index));
setSelectedRowKeys(newKeys);
},
```
```suggestion
const shouldSelect =
value !== undefined ? value :
!selectedRowKeys.includes(String(index));
const newKeys = shouldSelect
? [...selectedRowKeys, String(index)]
: selectedRowKeys.filter(k => k !== String(index));
```
##########
superset-frontend/src/components/ListView/useProTableState.ts:
##########
@@ -0,0 +1,336 @@
+/**
+ * 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.
+ */
+
+/**
+ * A simplified state management hook for ProTable that replaces the heavy
+ * react-table based useListViewState. This hook manages pagination, sorting,
+ * filtering, and view mode state while syncing with URL query params.
+ */
+
+import { useEffect, useMemo, useState, useCallback } from 'react';
+import {
+ NumberParam,
+ StringParam,
+ useQueryParams,
+ QueryParamConfig,
+} from 'use-query-params';
+import rison from 'rison';
+import {
+ ListViewFetchDataConfig as FetchDataConfig,
+ ListViewFilter as Filter,
+ ListViewFilterValue as FilterValue,
+ InnerFilterValue,
+ InternalFilter,
+ SortColumn,
+ ViewModeType,
+} from './types';
+
+// Custom RisonParam for proper encoding/decoding
+const RisonParam: QueryParamConfig<string, Record<string, InnerFilterValue>> =
{
+ encode: (data?: Record<string, InnerFilterValue> | null) => {
+ if (data === undefined || data === null) return undefined;
+
+ const cleanData = JSON.parse(
+ JSON.stringify(data, (_, value) => (value === undefined ? null : value)),
+ );
+
+ return rison
+ .encode(cleanData)
+ .replace(/%/g, '%25')
+ .replace(/&/g, '%26')
+ .replace(/\+/g, '%2B')
+ .replace(/#/g, '%23');
+ },
+ decode: (dataStr?: string | string[]) =>
+ dataStr === undefined || Array.isArray(dataStr)
+ ? undefined
+ : rison.decode(dataStr),
+};
+
+export interface UseProTableStateConfig<T extends object> {
+ fetchData: (conf: FetchDataConfig) => void;
+ data: T[];
+ count: number;
+ initialPageSize: number;
+ initialSort?: SortColumn[];
+ initialFilters?: Filter[];
+ bulkSelectMode?: boolean;
+ renderCard?: boolean;
+ defaultViewMode?: ViewModeType;
+}
+
+export interface ProTableState<T extends object> {
+ // Pagination
+ pageIndex: number;
+ pageSize: number;
+ pageCount: number;
+ gotoPage: (page: number) => void;
+
+ // Sorting
+ sortBy: SortColumn[];
+ setSortBy: (sortBy: SortColumn[]) => void;
+
+ // Filtering
+ internalFilters: InternalFilter[];
+ applyFilterValue: (index: number, value: InnerFilterValue) => void;
+
+ // View mode
+ viewMode: ViewModeType;
+ setViewMode: (mode: ViewModeType) => void;
+
+ // Selection
+ selectedRowKeys: string[];
+ setSelectedRowKeys: (keys: string[]) => void;
+ selectedRows: T[];
+ toggleAllRowsSelected: (selected: boolean) => void;
+
+ // Query state for empty state detection
+ query: {
+ filters?: Record<string, InnerFilterValue>;
+ pageIndex?: number | null;
+ sortColumn?: string | null;
+ sortOrder?: string | null;
+ viewMode?: string | null;
+ };
+}
+
+function mergeCreateFilterValues(
+ list: Filter[],
+ updateObj: Record<string, InnerFilterValue>,
+): InternalFilter[] {
+ return list.map(({ id, urlDisplay, operator }) => {
+ const currentFilterId = urlDisplay || id;
+ const update = updateObj[currentFilterId];
+ return { id, urlDisplay, operator, value: update };
+ });
+}
+
+function convertFilters(fts: InternalFilter[]): FilterValue[] {
+ return fts
+ .filter(
+ f =>
+ !(
+ typeof f.value === 'undefined' ||
+ (Array.isArray(f.value) && !f.value.length)
+ ),
+ )
+ .map(({ value, operator, id }) => {
+ if (operator === 'between' && Array.isArray(value)) {
+ return [
+ { value: value[0], operator: 'gt', id },
+ { value: value[1], operator: 'lt', id },
+ ];
+ }
+ return { value, operator, id };
+ })
+ .flat();
+}
+
+export function useProTableState<T extends object>({
+ fetchData,
+ data,
+ count,
+ initialPageSize,
+ initialFilters = [],
+ initialSort = [],
+ renderCard = false,
+ defaultViewMode = 'card',
+}: UseProTableStateConfig<T>): ProTableState<T> {
+ // URL query params
+ const [query, setQuery] = useQueryParams({
+ filters: RisonParam,
+ pageIndex: NumberParam,
+ sortColumn: StringParam,
+ sortOrder: StringParam,
+ viewMode: StringParam,
+ });
+
+ // Pagination state
+ const [pageIndex, setPageIndex] = useState<number>(query.pageIndex || 0);
+ const [pageSize] = useState<number>(initialPageSize);
+ const pageCount = useMemo(
+ () => Math.ceil(count / initialPageSize),
+ [count, initialPageSize],
+ );
+
+ // Sorting state
+ const [sortBy, setSortByState] = useState<SortColumn[]>(() => {
+ if (query.sortColumn && query.sortOrder) {
+ return [{ id: query.sortColumn, desc: query.sortOrder === 'desc' }];
+ }
+ return initialSort;
+ });
+
+ // Filter state
+ const [internalFilters, setInternalFilters] = useState<InternalFilter[]>(
+ () =>
+ query.filters && initialFilters.length
+ ? mergeCreateFilterValues(initialFilters, query.filters)
+ : [],
+ );
+
+ // View mode state
+ const [viewMode, setViewMode] = useState<ViewModeType>(
+ (query.viewMode as ViewModeType) ||
+ (renderCard ? defaultViewMode : 'table'),
+ );
+
+ // Selection state
+ const [selectedRowKeys, setSelectedRowKeys] = useState<string[]>([]);
+
+ // Computed selected rows
+ const selectedRows = useMemo(
+ () => data.filter((_, index) => selectedRowKeys.includes(String(index))),
+ [data, selectedRowKeys],
+ );
+
+ // Initialize filters when initialFilters changes
+ useEffect(() => {
+ if (initialFilters.length) {
+ setInternalFilters(
+ mergeCreateFilterValues(
+ initialFilters,
+ query.filters ? query.filters : {},
+ ),
+ );
+ }
+ }, [initialFilters, query.filters]);
+
+ // Sync state to URL and trigger fetch
+ useEffect(() => {
+ const filterObj: Record<string, InnerFilterValue> = {};
+
+ internalFilters.forEach(filter => {
+ if (
+ filter.value !== undefined &&
+ (typeof filter.value !== 'string' || filter.value.length > 0)
+ ) {
+ const currentFilterId = filter.urlDisplay || filter.id;
+ filterObj[currentFilterId] = filter.value;
+ }
+ });
+
+ const queryParams: {
+ filters?: Record<string, InnerFilterValue>;
+ pageIndex: number;
+ sortColumn?: string;
+ sortOrder?: string;
+ viewMode?: string;
+ } = {
+ filters: Object.keys(filterObj).length ? filterObj : undefined,
+ pageIndex,
+ };
+
+ if (sortBy?.[0]?.id !== undefined && sortBy[0].id !== null) {
+ queryParams.sortColumn = sortBy[0].id;
+ queryParams.sortOrder = sortBy[0].desc ? 'desc' : 'asc';
+ }
+
+ if (renderCard) {
+ queryParams.viewMode = viewMode;
+ }
+
+ const method =
+ typeof query.pageIndex !== 'undefined' &&
+ queryParams.pageIndex !== query.pageIndex
+ ? 'push'
+ : 'replace';
+
+ setQuery(queryParams, method);
+
+ // Trigger data fetch
+ fetchData({
+ pageIndex,
+ pageSize,
+ sortBy,
+ filters: convertFilters(internalFilters),
+ });
+ }, [pageIndex, pageSize, sortBy, internalFilters, viewMode, renderCard]);
Review Comment:
Missing dependencies in useEffect hook. The effect depends on `fetchData`,
`setQuery`, and `query` but these are not included in the dependency array.
This could lead to stale closures and incorrect behavior.
Add `fetchData`, `setQuery`, and `query` to the dependency array, or use
`useCallback` to memoize `fetchData` if it's passed as a prop.
```suggestion
}, [pageIndex, pageSize, sortBy, internalFilters, viewMode, renderCard,
setQuery, fetchData, query]);
```
##########
docs/package.json:
##########
@@ -28,6 +28,7 @@
},
"dependencies": {
"@ant-design/icons": "^6.1.0",
+ "@ant-design/pro-components" : "^2.8.10",
Review Comment:
[nitpick] Inconsistent spacing around the colon. The property name has an
extra space before the colon which is inconsistent with the formatting of other
dependencies in the file. Should be `"@ant-design/pro-components": "^2.8.10",`
without the space before the colon.
```suggestion
"@ant-design/pro-components": "^2.8.10",
```
##########
superset-frontend/src/components/ListView/useProTableState.ts:
##########
@@ -0,0 +1,336 @@
+/**
+ * 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.
+ */
+
+/**
+ * A simplified state management hook for ProTable that replaces the heavy
+ * react-table based useListViewState. This hook manages pagination, sorting,
+ * filtering, and view mode state while syncing with URL query params.
+ */
+
+import { useEffect, useMemo, useState, useCallback } from 'react';
+import {
+ NumberParam,
+ StringParam,
+ useQueryParams,
+ QueryParamConfig,
+} from 'use-query-params';
+import rison from 'rison';
+import {
+ ListViewFetchDataConfig as FetchDataConfig,
+ ListViewFilter as Filter,
+ ListViewFilterValue as FilterValue,
+ InnerFilterValue,
+ InternalFilter,
+ SortColumn,
+ ViewModeType,
+} from './types';
+
+// Custom RisonParam for proper encoding/decoding
+const RisonParam: QueryParamConfig<string, Record<string, InnerFilterValue>> =
{
+ encode: (data?: Record<string, InnerFilterValue> | null) => {
+ if (data === undefined || data === null) return undefined;
+
+ const cleanData = JSON.parse(
+ JSON.stringify(data, (_, value) => (value === undefined ? null : value)),
+ );
+
+ return rison
+ .encode(cleanData)
+ .replace(/%/g, '%25')
+ .replace(/&/g, '%26')
+ .replace(/\+/g, '%2B')
+ .replace(/#/g, '%23');
+ },
+ decode: (dataStr?: string | string[]) =>
+ dataStr === undefined || Array.isArray(dataStr)
+ ? undefined
+ : rison.decode(dataStr),
+};
+
+export interface UseProTableStateConfig<T extends object> {
+ fetchData: (conf: FetchDataConfig) => void;
+ data: T[];
+ count: number;
+ initialPageSize: number;
+ initialSort?: SortColumn[];
+ initialFilters?: Filter[];
+ bulkSelectMode?: boolean;
+ renderCard?: boolean;
+ defaultViewMode?: ViewModeType;
+}
+
+export interface ProTableState<T extends object> {
+ // Pagination
+ pageIndex: number;
+ pageSize: number;
+ pageCount: number;
+ gotoPage: (page: number) => void;
+
+ // Sorting
+ sortBy: SortColumn[];
+ setSortBy: (sortBy: SortColumn[]) => void;
+
+ // Filtering
+ internalFilters: InternalFilter[];
+ applyFilterValue: (index: number, value: InnerFilterValue) => void;
+
+ // View mode
+ viewMode: ViewModeType;
+ setViewMode: (mode: ViewModeType) => void;
+
+ // Selection
+ selectedRowKeys: string[];
+ setSelectedRowKeys: (keys: string[]) => void;
+ selectedRows: T[];
+ toggleAllRowsSelected: (selected: boolean) => void;
+
+ // Query state for empty state detection
+ query: {
+ filters?: Record<string, InnerFilterValue>;
+ pageIndex?: number | null;
+ sortColumn?: string | null;
+ sortOrder?: string | null;
+ viewMode?: string | null;
+ };
+}
+
+function mergeCreateFilterValues(
+ list: Filter[],
+ updateObj: Record<string, InnerFilterValue>,
+): InternalFilter[] {
+ return list.map(({ id, urlDisplay, operator }) => {
+ const currentFilterId = urlDisplay || id;
+ const update = updateObj[currentFilterId];
+ return { id, urlDisplay, operator, value: update };
+ });
+}
+
+function convertFilters(fts: InternalFilter[]): FilterValue[] {
+ return fts
+ .filter(
+ f =>
+ !(
+ typeof f.value === 'undefined' ||
+ (Array.isArray(f.value) && !f.value.length)
+ ),
+ )
+ .map(({ value, operator, id }) => {
+ if (operator === 'between' && Array.isArray(value)) {
+ return [
+ { value: value[0], operator: 'gt', id },
+ { value: value[1], operator: 'lt', id },
+ ];
+ }
+ return { value, operator, id };
+ })
+ .flat();
+}
+
+export function useProTableState<T extends object>({
+ fetchData,
+ data,
+ count,
+ initialPageSize,
+ initialFilters = [],
+ initialSort = [],
+ renderCard = false,
+ defaultViewMode = 'card',
+}: UseProTableStateConfig<T>): ProTableState<T> {
+ // URL query params
+ const [query, setQuery] = useQueryParams({
+ filters: RisonParam,
+ pageIndex: NumberParam,
+ sortColumn: StringParam,
+ sortOrder: StringParam,
+ viewMode: StringParam,
+ });
+
+ // Pagination state
+ const [pageIndex, setPageIndex] = useState<number>(query.pageIndex || 0);
+ const [pageSize] = useState<number>(initialPageSize);
+ const pageCount = useMemo(
+ () => Math.ceil(count / initialPageSize),
+ [count, initialPageSize],
+ );
+
+ // Sorting state
+ const [sortBy, setSortByState] = useState<SortColumn[]>(() => {
+ if (query.sortColumn && query.sortOrder) {
+ return [{ id: query.sortColumn, desc: query.sortOrder === 'desc' }];
+ }
+ return initialSort;
+ });
+
+ // Filter state
+ const [internalFilters, setInternalFilters] = useState<InternalFilter[]>(
+ () =>
+ query.filters && initialFilters.length
+ ? mergeCreateFilterValues(initialFilters, query.filters)
+ : [],
+ );
+
+ // View mode state
+ const [viewMode, setViewMode] = useState<ViewModeType>(
+ (query.viewMode as ViewModeType) ||
+ (renderCard ? defaultViewMode : 'table'),
+ );
+
+ // Selection state
+ const [selectedRowKeys, setSelectedRowKeys] = useState<string[]>([]);
+
+ // Computed selected rows
+ const selectedRows = useMemo(
+ () => data.filter((_, index) => selectedRowKeys.includes(String(index))),
+ [data, selectedRowKeys],
+ );
+
+ // Initialize filters when initialFilters changes
+ useEffect(() => {
+ if (initialFilters.length) {
+ setInternalFilters(
+ mergeCreateFilterValues(
+ initialFilters,
+ query.filters ? query.filters : {},
+ ),
+ );
+ }
+ }, [initialFilters, query.filters]);
+
+ // Sync state to URL and trigger fetch
+ useEffect(() => {
+ const filterObj: Record<string, InnerFilterValue> = {};
+
+ internalFilters.forEach(filter => {
+ if (
+ filter.value !== undefined &&
+ (typeof filter.value !== 'string' || filter.value.length > 0)
+ ) {
+ const currentFilterId = filter.urlDisplay || filter.id;
+ filterObj[currentFilterId] = filter.value;
+ }
+ });
+
+ const queryParams: {
+ filters?: Record<string, InnerFilterValue>;
+ pageIndex: number;
+ sortColumn?: string;
+ sortOrder?: string;
+ viewMode?: string;
+ } = {
+ filters: Object.keys(filterObj).length ? filterObj : undefined,
+ pageIndex,
+ };
+
+ if (sortBy?.[0]?.id !== undefined && sortBy[0].id !== null) {
+ queryParams.sortColumn = sortBy[0].id;
+ queryParams.sortOrder = sortBy[0].desc ? 'desc' : 'asc';
+ }
+
+ if (renderCard) {
+ queryParams.viewMode = viewMode;
+ }
+
+ const method =
+ typeof query.pageIndex !== 'undefined' &&
+ queryParams.pageIndex !== query.pageIndex
+ ? 'push'
+ : 'replace';
+
+ setQuery(queryParams, method);
+
+ // Trigger data fetch
+ fetchData({
+ pageIndex,
+ pageSize,
+ sortBy,
+ filters: convertFilters(internalFilters),
+ });
+ }, [pageIndex, pageSize, sortBy, internalFilters, viewMode, renderCard]);
+
+ // Reset to first page if current page is invalid
+ useEffect(() => {
+ if (pageIndex > pageCount - 1 && pageCount > 0) {
+ setPageIndex(0);
+ }
+ }, [pageCount, pageIndex]);
+
+ // Sync from URL on initial load
+ useEffect(() => {
+ if (query.pageIndex !== undefined && query.pageIndex !== pageIndex) {
+ setPageIndex(query.pageIndex);
+ }
Review Comment:
[nitpick] The useEffect for syncing from URL on initial load is missing
`query.pageIndex` and `pageIndex` in its dependency array. This effect should
only run once on mount, so the empty dependency array is correct, but ESLint
will warn about this. Consider adding an ESLint disable comment to clarify the
intent:
```typescript
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
```
```suggestion
}
// eslint-disable-next-line react-hooks/exhaustive-deps
```
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -448,52 +567,61 @@ export function ListView<T extends object = any>({
{viewMode === 'card' && (
<CardCollection
bulkSelectEnabled={bulkSelectEnabled}
- prepareRow={prepareRow}
+ prepareRow={() => {}}
renderCard={renderCard}
- rows={rows}
+ rows={cardRows as never}
loading={loading}
showThumbnails={showThumbnails}
/>
)}
{viewMode === 'table' && (
<>
- {loading && rows.length === 0 ? (
+ {loading && dataWithKeys.length === 0 ? (
<FullPageLoadingWrapper>
<Loading />
</FullPageLoadingWrapper>
) : (
- <TableCollection
- getTableProps={getTableProps}
- getTableBodyProps={getTableBodyProps}
- prepareRow={prepareRow}
- headerGroups={headerGroups}
- setSortBy={setSortBy}
- rows={rows}
- columns={columns}
- loading={loading && rows.length > 0}
- highlightRowId={highlightRowId}
- columnsForWrapText={columnsForWrapText}
- bulkSelectEnabled={bulkSelectEnabled}
- selectedFlatRows={selectedFlatRows}
- toggleRowSelected={(rowId, value) => {
- const row = rows.find((r: any) => r.id === rowId);
- if (row) {
- prepareRow(row);
- (row as any).toggleRowSelected(value);
- }
- }}
- toggleAllRowsSelected={toggleAllRowsSelected}
- pageIndex={pageIndex}
- pageSize={pageSize}
- totalCount={count}
- onPageChange={newPageIndex => {
- gotoPage(newPageIndex);
- }}
- />
+ <ConfigProvider locale={enUS}>
+ <ProTable
+ data-test="listview-table"
+ actionRef={actionRef}
+ columns={proColumns as ProColumns[]}
Review Comment:
[nitpick] Improper indentation. The `columns` prop on line 588 should be
aligned with the other props starting on line 586.
```suggestion
columns={proColumns as ProColumns[]}
```
##########
superset-frontend/src/components/ListView/useProTableState.ts:
##########
@@ -0,0 +1,336 @@
+/**
+ * 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.
+ */
+
+/**
+ * A simplified state management hook for ProTable that replaces the heavy
+ * react-table based useListViewState. This hook manages pagination, sorting,
+ * filtering, and view mode state while syncing with URL query params.
Review Comment:
[nitpick] The documentation says this hook "replaces the heavy react-table
based useListViewState", but it doesn't mention the key architectural change:
this hook uses array indices as row keys instead of actual row IDs. This is a
significant behavior change that should be documented, especially since it
affects row selection persistence across pagination.
```suggestion
* filtering, and view mode state while syncing with URL query params.
*
* Key architectural change: This hook uses array indices as row keys
instead of actual row IDs.
* This affects row selection persistence across pagination—selected rows
are tracked by their
* position in the current page's data array, not by a unique identifier. If
the data changes
* or the page is changed, selection may not persist as expected. Developers
should be aware
* of this difference when migrating from the previous react-table based
implementation.
```
##########
superset-frontend/src/components/ListView/useProTableState.ts:
##########
@@ -0,0 +1,336 @@
+/**
+ * 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.
+ */
+
+/**
+ * A simplified state management hook for ProTable that replaces the heavy
+ * react-table based useListViewState. This hook manages pagination, sorting,
+ * filtering, and view mode state while syncing with URL query params.
+ */
+
+import { useEffect, useMemo, useState, useCallback } from 'react';
+import {
+ NumberParam,
+ StringParam,
+ useQueryParams,
+ QueryParamConfig,
+} from 'use-query-params';
+import rison from 'rison';
+import {
+ ListViewFetchDataConfig as FetchDataConfig,
+ ListViewFilter as Filter,
+ ListViewFilterValue as FilterValue,
+ InnerFilterValue,
+ InternalFilter,
+ SortColumn,
+ ViewModeType,
+} from './types';
+
+// Custom RisonParam for proper encoding/decoding
+const RisonParam: QueryParamConfig<string, Record<string, InnerFilterValue>> =
{
+ encode: (data?: Record<string, InnerFilterValue> | null) => {
+ if (data === undefined || data === null) return undefined;
+
+ const cleanData = JSON.parse(
+ JSON.stringify(data, (_, value) => (value === undefined ? null : value)),
+ );
+
+ return rison
+ .encode(cleanData)
+ .replace(/%/g, '%25')
+ .replace(/&/g, '%26')
+ .replace(/\+/g, '%2B')
+ .replace(/#/g, '%23');
+ },
+ decode: (dataStr?: string | string[]) =>
+ dataStr === undefined || Array.isArray(dataStr)
+ ? undefined
+ : rison.decode(dataStr),
+};
+
+export interface UseProTableStateConfig<T extends object> {
+ fetchData: (conf: FetchDataConfig) => void;
+ data: T[];
+ count: number;
+ initialPageSize: number;
+ initialSort?: SortColumn[];
+ initialFilters?: Filter[];
+ bulkSelectMode?: boolean;
+ renderCard?: boolean;
+ defaultViewMode?: ViewModeType;
+}
+
+export interface ProTableState<T extends object> {
+ // Pagination
+ pageIndex: number;
+ pageSize: number;
+ pageCount: number;
+ gotoPage: (page: number) => void;
+
+ // Sorting
+ sortBy: SortColumn[];
+ setSortBy: (sortBy: SortColumn[]) => void;
+
+ // Filtering
+ internalFilters: InternalFilter[];
+ applyFilterValue: (index: number, value: InnerFilterValue) => void;
+
+ // View mode
+ viewMode: ViewModeType;
+ setViewMode: (mode: ViewModeType) => void;
+
+ // Selection
+ selectedRowKeys: string[];
+ setSelectedRowKeys: (keys: string[]) => void;
+ selectedRows: T[];
+ toggleAllRowsSelected: (selected: boolean) => void;
+
+ // Query state for empty state detection
+ query: {
+ filters?: Record<string, InnerFilterValue>;
+ pageIndex?: number | null;
+ sortColumn?: string | null;
+ sortOrder?: string | null;
+ viewMode?: string | null;
+ };
+}
+
+function mergeCreateFilterValues(
+ list: Filter[],
+ updateObj: Record<string, InnerFilterValue>,
+): InternalFilter[] {
+ return list.map(({ id, urlDisplay, operator }) => {
+ const currentFilterId = urlDisplay || id;
+ const update = updateObj[currentFilterId];
+ return { id, urlDisplay, operator, value: update };
+ });
+}
+
+function convertFilters(fts: InternalFilter[]): FilterValue[] {
+ return fts
+ .filter(
+ f =>
+ !(
+ typeof f.value === 'undefined' ||
+ (Array.isArray(f.value) && !f.value.length)
+ ),
+ )
+ .map(({ value, operator, id }) => {
+ if (operator === 'between' && Array.isArray(value)) {
+ return [
+ { value: value[0], operator: 'gt', id },
+ { value: value[1], operator: 'lt', id },
+ ];
+ }
+ return { value, operator, id };
+ })
+ .flat();
+}
+
+export function useProTableState<T extends object>({
+ fetchData,
+ data,
+ count,
+ initialPageSize,
+ initialFilters = [],
+ initialSort = [],
+ renderCard = false,
+ defaultViewMode = 'card',
+}: UseProTableStateConfig<T>): ProTableState<T> {
+ // URL query params
+ const [query, setQuery] = useQueryParams({
+ filters: RisonParam,
+ pageIndex: NumberParam,
+ sortColumn: StringParam,
+ sortOrder: StringParam,
+ viewMode: StringParam,
+ });
+
+ // Pagination state
+ const [pageIndex, setPageIndex] = useState<number>(query.pageIndex || 0);
+ const [pageSize] = useState<number>(initialPageSize);
+ const pageCount = useMemo(
+ () => Math.ceil(count / initialPageSize),
+ [count, initialPageSize],
+ );
+
+ // Sorting state
+ const [sortBy, setSortByState] = useState<SortColumn[]>(() => {
+ if (query.sortColumn && query.sortOrder) {
+ return [{ id: query.sortColumn, desc: query.sortOrder === 'desc' }];
+ }
+ return initialSort;
+ });
+
+ // Filter state
+ const [internalFilters, setInternalFilters] = useState<InternalFilter[]>(
+ () =>
+ query.filters && initialFilters.length
+ ? mergeCreateFilterValues(initialFilters, query.filters)
+ : [],
+ );
+
+ // View mode state
+ const [viewMode, setViewMode] = useState<ViewModeType>(
+ (query.viewMode as ViewModeType) ||
+ (renderCard ? defaultViewMode : 'table'),
+ );
+
+ // Selection state
+ const [selectedRowKeys, setSelectedRowKeys] = useState<string[]>([]);
+
+ // Computed selected rows
+ const selectedRows = useMemo(
+ () => data.filter((_, index) => selectedRowKeys.includes(String(index))),
+ [data, selectedRowKeys],
+ );
Review Comment:
The `selectedRows` computation uses array indices as row keys, which may
cause issues if the data changes between pages or after filtering. When the
user navigates to a different page, the indices reset, but `selectedRowKeys`
still contains the old string indices, causing incorrect rows to appear
selected.
Consider using a unique identifier from the row data (e.g., `row.id`)
instead of array index for row keys to maintain correct selection across
pagination and filtering.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
