You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rkc9937 (via GitHub)" <gi...@apache.org> on 2025/11/30 20:04:48 UTC
[PR] refactor: replace ListView with Ant Design ProTable [superset]
rkc9937 opened a new pull request, #36345:
URL: https://github.com/apache/superset/pull/36345
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
This proof-of-concept replaces Superset’s custom list-view table implementation with [Ant Design Pro Table](https://procomponents.ant.design/en-US/components/table), reducing custom table logic and simplifying ListView by delegating table behavior (pagination, sorting, filtering, selection) to a well-supported component.
### TESTING INSTRUCTIONS
1. Navigate to a ListView-based page (e.g., Dashboards, Charts, Datasets).
2. Verify:
- Table renders with the expected columns and data.
- Pagination works and triggers server-side fetches.
- Clicking column headers updates sort order and URL parameters.
- External filters still apply correctly and affect results + URL.
- Bulk selection, bulk actions, and bulk tagging work as before.
- Toggling between card/table view works and persists in URL.
- Empty states show the correct messaging and “clear all filters” behavior.
3. Hard-refresh the page and confirm the list view restores its state from the URL.
### ADDITIONAL INFORMATION
### Why Pro Table?
The current ListView table includes a lot of custom logic — manual pagination, sorting, filtering, selection, and URL syncing. Ant Design Pro Table already provides these capabilities out of the box, fits naturally with Superset’s existing Ant Design stack, and avoids the licensing issues we ran into with other table libraries.
Moving to Pro Table removes a large amount of bespoke table code and lets ListView focus on data and state rather than re-implementing table behavior.
### What Changed
#### `ListView.tsx`
- Replaces the `react-table` implementation with `ProTable`.
- Converts existing column definitions into Pro Table’s format.
- Integrates pagination, sorting, filtering, and row selection through the new state hook.
- Preserves all existing ListView UX features:
- external filters (`FilterControls`)
- bulk selection and bulk tagging
- row highlighting
- card/table view toggle
- loading and empty states
#### `useProTableState.ts` (new)
- Centralizes all ListView state: pagination, sorting, filters, selection.
- Syncs this state with the URL using `use-query-params` and Rison.
- Triggers server-side fetches whenever state changes.
- Works seamlessly with both table and card views and keeps the existing filter structure.
---
### Architecture Notes
This POC significantly cuts down on custom table code inside ListView and moves that responsibility to a component already aligned with Superset’s UI choices. The user experience stays the same, while the architecture becomes easier to maintain and extend — making features like built-in Pro Table filters, shared state hooks across list views, or a refreshed card view design more achievable in the future.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36345:
URL: https://github.com/apache/superset/pull/36345#issuecomment-3593249858
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36345:
URL: https://github.com/apache/superset/pull/36345#issuecomment-3598392615
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19834110018) for [bba1f87](https://github.com/apache/superset/commit/bba1f87)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36345:
URL: https://github.com/apache/superset/pull/36345#issuecomment-3598485579
Looks like `pre-commit` needs to be run to make it mergeable, too.
In general, this POC is looking GREAT. If we can get CI to pass, and address those comments (mine and Copilot's) I think we can test this on an ephemeral, and hopefully merge it if it looks/acts as good as it sounds!
Thanks for taking this on, it's REALLY promising!!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36345:
URL: https://github.com/apache/superset/pull/36345#issuecomment-3604147302
Re-running CI to see what we find there 🤞
We'll keep looking at this and will give it a test. Very exciting! Feel free to reply to the copilot comments and let us know if they're worth consideration (or... not so much)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36345:
URL: https://github.com/apache/superset/pull/36345#issuecomment-4662004721
I remain excited about this POC - it looks really promising @rkc9937!
To move it forward: please rebase (branch is conflicting), run `pre-commit run --all-files` to make it mergeable, add tests for the listview behavior, and reply to the Copilot/maintainer comments — including which existing components this lets us remove. Then we'll test on an ephemeral and aim to merge.
I'll set this to `draft` for now, while it awaits some patch-up, but let me know if you need help to drive it to completion... I might be able to pick it up if you're not able to, otherwise it might eventually be closed out and referred to later in another effort... I still believe it's a worthwhile endeavor!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "rkc9937 (via GitHub)" <gi...@apache.org>.
rkc9937 commented on PR #36345:
URL: https://github.com/apache/superset/pull/36345#issuecomment-3593274018
hey guys, just want to say that this is a proof of concept (poc) - and in the future, we would love to reduce custom code for other components. any feedback or comments would be much appreciated.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36345:
URL: https://github.com/apache/superset/pull/36345#issuecomment-3598387927
It looks like there's a fair bit of custom logic going on here... we'll read through and see if any of it seems like it could be simplified.
Would it be possible to add some tests? This is definitely a core experience we don't want regressing.
I'll spin up an ephemeral environment so we can all test.
Last but not least, does this allow us to delete a bunch of the existing UI code/components in favor of this approach?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor: replace ListView with Ant Design ProTable [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org