feat(table): virtualization #4285
Conversation
🦋 Changeset detectedLatest commit: 455149b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this pull request involve updates to the documentation and functionality of various table components within the NextUI framework. Key modifications include the introduction of virtualization features to enhance performance when rendering large datasets. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
vinroger
force-pushed
the
feat/eng-1633-virtualization-for-table
branch
2 times, most recently
from
6346107 to
9f83094
Compare
vinroger
force-pushed
the
feat/eng-1633-virtualization-for-table
branch
from
9f83094 to
ed0a7d8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (9)
packages/components/table/src/virtualized-table.tsx (1)
71-75: Update the dependency array inuseLayoutEffectCurrently,
useLayoutEffectdepends onheaderRef, which is a ref object whose.currentproperty may change without changing the ref itself. To ensureheaderHeightupdates correctly whenheaderRef.currentchanges, consider updating the effect:Apply this diff to adjust the effect:
useLayoutEffect(() => { if (headerRef.current) { setHeaderHeight(headerRef.current.getBoundingClientRect().height); } - }, [headerRef]); + }, []);Alternatively, if the
headerHeightmight change due to dynamic content, consider using a ResizeObserver.apps/docs/content/components/table/virtualization.raw.jsx (1)
3-9: Consider memoizing row generation for large datasetsWhile the current implementation is clean and straightforward, consider memoizing the generated rows using
useMemoin the parent component to prevent unnecessary regeneration on re-renders.+import {useMemo} from "react"; function generateRows(count) { return Array.from({length: count}, (_, index) => ({ key: index.toString(), name: `Item ${index + 1}`, value: `Value ${index + 1}`, })); } export default function App() { - const rows = generateRows(500); + const rows = useMemo(() => generateRows(500), []);apps/docs/content/components/table/virtualization-custom-row-height.raw.jsx (1)
19-24: Document performance implications of row heightWhen using larger row heights with virtualization, consider adding a comment about the impact on the visible row count and overall performance. Also, consider implementing windowing optimization.
<Table isVirtualized aria-label="Example of virtualized table with a large dataset" maxTableHeight={500} rowHeight={70} + // With 70px row height and 500px max height, approximately 7 rows will be visible + // Consider adjusting overscan to optimize rendering + overscanCount={2} >apps/docs/content/components/table/virtualization-ten-thousand.raw.jsx (3)
11-17: Add error boundary for large dataset handlingLarge datasets can potentially cause rendering issues. Consider adding an error boundary to gracefully handle any rendering failures.
+class TableErrorBoundary extends React.Component { + state = { hasError: false }; + + static getDerivedStateFromError(error) { + return { hasError: true }; + } + + render() { + if (this.state.hasError) { + return <div>Error loading table data. Please try again.</div>; + } + return this.props.children; + } +} export default function App() { const rows = generateRows(10000); // ... return ( + <TableErrorBoundary> <Table> {/* ... */} </Table> + </TableErrorBoundary> ); }
19-24: Consider responsive table heightThe fixed
maxTableHeightmight not be optimal for all viewport sizes. Consider making it responsive or container-based.+import {useCallback} from "react"; + +function useResponsiveHeight() { + const getHeight = useCallback(() => { + // Consider viewport height or container size + return Math.min(500, window.innerHeight * 0.7); + }, []); + + const [height, setHeight] = useState(getHeight()); + + useEffect(() => { + const handleResize = () => setHeight(getHeight()); + window.addEventListener('resize', handleResize); + return () => window.removeEventListener('resize', handleResize); + }, [getHeight]); + + return height; +} export default function App() { + const tableHeight = useResponsiveHeight(); return ( <Table isVirtualized aria-label="Example of virtualized table with a large dataset" - maxTableHeight={500} + maxTableHeight={tableHeight} rowHeight={40} >
28-34: Add loading state for initial renderLarge datasets might cause a noticeable delay during the initial render. Consider adding a loading state.
+import {Spinner} from "@nextui-org/react"; + export default function App() { + const [isLoading, setIsLoading] = useState(true); const rows = useMemo(() => { const data = generateRows(10000); + setIsLoading(false); return data; }, []); return ( <Table> - <TableBody items={rows}> + <TableBody + items={rows} + loadingContent={<Spinner />} + isLoading={isLoading} + > {(item) => ( <TableRow key={item.key}> {(columnKey) => <TableCell>{item[columnKey]}</TableCell>} </TableRow> )} </TableBody> </Table> ); }packages/components/table/src/table.tsx (1)
39-41: Consider adjusting the virtualization threshold.The current implementation enables virtualization when collection size exceeds 50 items. Consider making this threshold configurable via props to give users more control over when virtualization kicks in.
-const shouldVirtualize = values.collection.size > 50 || isVirtualized; +const virtualizationThreshold = props.virtualizationThreshold ?? 50; +const shouldVirtualize = values.collection.size > virtualizationThreshold || isVirtualized;packages/components/table/stories/table.stories.tsx (2)
1114-1120: Add TypeScript types for better type safety.The
generateRowsfunction would benefit from explicit TypeScript types for its return value.-function generateRows(count) { +interface GeneratedRow { + key: string; + name: string; + value: string; +} + +function generateRows(count: number): GeneratedRow[] {
1132-1138: Add error boundaries for virtualized tables.The virtualized table implementation should include error boundaries to gracefully handle potential rendering issues with large datasets.
+class VirtualizedTableErrorBoundary extends React.Component { + // ... error boundary implementation +} + <Table aria-label="Example of virtualized table with a large dataset" {...args} isVirtualized maxTableHeight={300} rowHeight={40} + ErrorBoundary={VirtualizedTableErrorBoundary} >Also applies to: 1169-1175
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
apps/docs/config/routes.json(1 hunks)apps/docs/content/components/table/index.ts(2 hunks)apps/docs/content/components/table/virtualization-custom-max-table-height.raw.jsx(1 hunks)apps/docs/content/components/table/virtualization-custom-max-table-height.ts(1 hunks)apps/docs/content/components/table/virtualization-custom-row-height.raw.jsx(1 hunks)apps/docs/content/components/table/virtualization-custom-row-height.ts(1 hunks)apps/docs/content/components/table/virtualization-ten-thousand.raw.jsx(1 hunks)apps/docs/content/components/table/virtualization-ten-thousand.ts(1 hunks)apps/docs/content/components/table/virtualization.raw.jsx(1 hunks)apps/docs/content/components/table/virtualization.ts(1 hunks)apps/docs/content/docs/components/table.mdx(2 hunks)packages/components/table/package.json(1 hunks)packages/components/table/src/table-row-group.tsx(1 hunks)packages/components/table/src/table.tsx(3 hunks)packages/components/table/src/virtualized-table-body.tsx(1 hunks)packages/components/table/src/virtualized-table.tsx(1 hunks)packages/components/table/stories/table.stories.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/docs/content/components/table/virtualization-custom-max-table-height.ts
- apps/docs/content/components/table/virtualization-ten-thousand.ts
- apps/docs/content/components/table/virtualization-custom-row-height.ts
🔇 Additional comments (15)
packages/components/table/src/virtualized-table-body.tsx (1)
1-169: Overall implementation of VirtualizedTableBody is solid
The VirtualizedTableBody component is well-structured and properly handles virtualization of table rows using @tanstack/react-virtual. Good job on managing loading and empty states effectively.
packages/components/table/src/virtualized-table.tsx (1)
20-151: VirtualizedTable component is well-implemented
The VirtualizedTable component effectively utilizes useVirtualizer to enhance performance through virtualization. The structure is clean, and the code follows best practices.
apps/docs/content/components/table/virtualization.ts (2)
1-1: Verify the usage of ?raw in the import statement
The ?raw suffix imports the raw content of the file as a string. However, you are using App as if it were a component. Ensure this is intentional and that App is handled correctly in the exported object.
3-9: Review the export structure for consistency
The exported object contains the key "/App.jsx" with App as its value. Confirm that this aligns with how the content is consumed elsewhere, especially considering the potential raw import.
apps/docs/content/components/table/virtualization-custom-row-height.raw.jsx (1)
28-34: Optimize rendering for large datasets
Since this example demonstrates a larger row height, consider implementing progressive loading or infinite scroll for better performance with large datasets.
Let's verify the current implementation's performance:
packages/components/table/package.json (1)
56-57: Verify @tanstack/react-virtual version specification
The specified version ^3.10.9 seems unusual as the latest stable version follows a different pattern. Please verify:
- If this specific version is required
- If we should align with the latest stable version
✅ Verification successful
Version ^3.10.9 is valid but can be updated to latest stable
The specified version is a valid release, but there's a newer stable version (3.11.0) available. The caret (^) in the version specification will allow updates to minor versions, so updating to ^3.11.0 would be recommended to get the latest features and bug fixes while maintaining compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check npm registry for package versions
curl -s https://registry.npmjs.org/@tanstack/react-virtual | jq -r '.["dist-tags"].latest'
# Check for potential breaking changes
npm view @tanstack/react-virtual versions --json | jq -r '.[]' | grep '^3\.'
Length of output: 1198
apps/docs/content/components/table/index.ts (1)
22-25: LGTM! Well-organized virtualization feature exports
The new imports and exports are well-structured, following the existing pattern and using descriptive names.
Also applies to: 49-52
packages/components/table/src/table.tsx (3)
6-6: LGTM: Clean import of VirtualizedTable component.
The import statement is correctly placed and follows the project's import style.
13-18: LGTM: Well-structured interface extension.
The TableProps interface is properly extended with:
- Generic type support for better type safety
- Clear virtualization-related props with appropriate types
54-63: LGTM: Clean virtualization implementation.
The conditional rendering of VirtualizedTable is well implemented with proper prop forwarding.
apps/docs/config/routes.json (1)
413-414: LGTM: Route correctly marked as updated.
The table component's route is properly marked as updated to reflect the new virtualization feature.
apps/docs/content/docs/components/table.mdx (3)
321-331: LGTM: Clear introduction to virtualization feature.
The documentation clearly explains the virtualization feature and its benefits. The note about @tanstack/react-virtual is helpful for users who want to understand the underlying implementation.
332-348: LGTM: Comprehensive examples.
The documentation includes well-structured examples covering:
- Large dataset (10,000 items)
- Custom max table height
- Custom row height
489-506: LGTM: Well-documented API props.
The new virtualization-related props are properly documented with clear descriptions and default values.
packages/components/table/stories/table.stories.tsx (1)
1124-1124: Consider lazy loading for large datasets.
Generating large arrays synchronously (500 and 10,000 rows) might impact initial load performance. Consider implementing lazy loading or chunked data generation.
Also applies to: 1161-1161
| transform: `translateY(${virtualRow.start - index * virtualRow.size}px)`, | ||
| height: `${virtualRow.size}px`, |
There was a problem hiding this comment.
Correct the transform calculation for proper row positioning
The transform property in the row's style should use virtualRow.start directly. Subtracting index * virtualRow.size can lead to incorrect row positioning.
Apply this diff to fix the calculation:
- transform: `translateY(${virtualRow.start - index * virtualRow.size}px)`,
+ transform: `translateY(${virtualRow.start}px)`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transform: `translateY(${virtualRow.start - index * virtualRow.size}px)`, | |
| height: `${virtualRow.size}px`, | |
| transform: `translateY(${virtualRow.start}px)`, | |
| height: `${virtualRow.size}px`, |
| @@ -11,7 +11,7 @@ export interface TableRowGroupProps extends HTMLNextUIProps<"thead"> { | |||
| classNames?: ValuesType["classNames"]; | |||
| } | |||
|
|
|||
| const TableRowGroup = forwardRef<"thead", TableRowGroupProps>((props, ref) => { | |||
| const TableRowGroup = forwardRef<any, TableRowGroupProps>((props, ref) => { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any for the ref type to maintain type safety
Changing the ref type from "thead" to any reduces type safety and may introduce bugs. Specify the correct HTML element type to ensure proper type-checking and autocomplete support.
Apply this diff to restore type safety:
- const TableRowGroup = forwardRef<any, TableRowGroupProps>((props, ref) => {
+ const TableRowGroup = forwardRef<HTMLTableSectionElement, TableRowGroupProps>((props, ref) => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TableRowGroup = forwardRef<any, TableRowGroupProps>((props, ref) => { | |
| const TableRowGroup = forwardRef<HTMLTableSectionElement, TableRowGroupProps>((props, ref) => { |
| <Table | ||
| isVirtualized | ||
| aria-label="Example of virtualized table with a large dataset" | ||
| maxTableHeight={500} | ||
| rowHeight={40} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and loading states
The table implementation could benefit from additional error handling and loading states to improve user experience.
<Table
isVirtualized
aria-label="Example of virtualized table with a large dataset"
maxTableHeight={500}
rowHeight={40}
+ loadingState={isLoading ? "loading" : "idle"}
+ loadingContent={<Spinner label="Loading..." />}
+ emptyContent={<div>No rows to display</div>}
>Committable suggestion skipped: line range outside the PR's diff.
| <TableBody items={rows}> | ||
| {(item) => ( | ||
| <TableRow key={item.key}> | ||
| {(columnKey) => <TableCell>{item[columnKey]}</TableCell>} | ||
| </TableRow> | ||
| )} | ||
| </TableBody> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add aria-rowindex for better screen reader support
When virtualizing table rows, it's important to maintain proper accessibility by indicating the absolute row position to screen readers.
<TableBody items={rows}>
{(item) => (
- <TableRow key={item.key}>
+ <TableRow
+ key={item.key}
+ aria-rowindex={parseInt(item.key) + 2} // +2 to account for header row and 1-based indexing
+ >
{(columnKey) => <TableCell>{item[columnKey]}</TableCell>}
</TableRow>
)}
</TableBody>| function generateRows(count) { | ||
| return Array.from({length: count}, (_, index) => ({ | ||
| key: index.toString(), | ||
| name: `Item ${index + 1}`, | ||
| value: `Value ${index + 1}`, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract generateRows to a shared utility file
This helper function is duplicated across multiple example files. Consider moving it to a shared utility file to improve maintainability.
Create a new file apps/docs/content/components/table/utils.js:
export function generateRows(count) {
return Array.from({length: count}, (_, index) => ({
key: index.toString(),
name: `Item ${index + 1}`,
value: `Value ${index + 1}`,
}));
}Then import it in each example file:
+import {generateRows} from "./utils";
-function generateRows(count) {
- // ... remove duplicated function
-}| function generateRows(count) { | ||
| return Array.from({length: count}, (_, index) => ({ | ||
| key: index.toString(), | ||
| name: `Item ${index + 1}`, | ||
| value: `Value ${index + 1}`, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing row generation
The generateRows function creates a new array on every render. Consider memoizing the result to prevent unnecessary re-renders.
+import {useMemo} from "react";
function generateRows(count) {
return Array.from({length: count}, (_, index) => ({
key: index.toString(),
name: `Item ${index + 1}`,
value: `Value ${index + 1}`,
}));
}
export default function App() {
- const rows = generateRows(10000);
+ const rows = useMemo(() => generateRows(10000), []);Committable suggestion skipped: line range outside the PR's diff.
| export const Virtualized = { | ||
| render: (args: TableProps) => { | ||
| const rows = generateRows(500); | ||
| const columns = [ | ||
| {key: "name", label: "Name"}, | ||
| {key: "value", label: "Value"}, | ||
| ]; | ||
|
|
||
| return ( | ||
| <div> | ||
| <Table | ||
| aria-label="Example of virtualized table with a large dataset" | ||
| {...args} | ||
| isVirtualized | ||
| maxTableHeight={300} | ||
| rowHeight={40} | ||
| > | ||
| <TableHeader columns={columns}> | ||
| {(column) => <TableColumn key={column.key}>{column.label}</TableColumn>} | ||
| </TableHeader> | ||
| <TableBody items={rows}> | ||
| {(item) => ( | ||
| <TableRow key={item.key}> | ||
| {(columnKey) => <TableCell>{item[columnKey]}</TableCell>} | ||
| </TableRow> | ||
| )} | ||
| </TableBody> | ||
| </Table> | ||
| </div> | ||
| ); | ||
| }, | ||
| args: { | ||
| ...defaultProps, | ||
| className: "max-w-3xl", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating duplicate story implementations.
The Virtualized and TenThousandRows stories share almost identical implementation, differing only in row count. Consider refactoring to reduce code duplication.
+const VirtualizedTableStory = ({rowCount, ...args}: TableProps & {rowCount: number}) => {
+ const rows = generateRows(rowCount);
+ const columns = [
+ {key: "name", label: "Name"},
+ {key: "value", label: "Value"},
+ ];
+
+ return (
+ <div>
+ <Table
+ aria-label={`Example of virtualized table with ${rowCount} rows`}
+ {...args}
+ isVirtualized
+ maxTableHeight={300}
+ rowHeight={40}
+ >
+ <TableHeader columns={columns}>
+ {(column) => <TableColumn key={column.key}>{column.label}</TableColumn>}
+ </TableHeader>
+ <TableBody items={rows}>
+ {(item) => (
+ <TableRow key={item.key}>
+ {(columnKey) => <TableCell>{item[columnKey]}</TableCell>}
+ </TableRow>
+ )}
+ </TableBody>
+ </Table>
+ </div>
+ );
+};
+
+export const Virtualized = {
+ render: (args: TableProps) => <VirtualizedTableStory rowCount={500} {...args} />,
+ args: {
+ ...defaultProps,
+ className: "max-w-3xl",
+ },
+};
+
+export const TenThousandRows = {
+ render: (args: TableProps) => <VirtualizedTableStory rowCount={10000} {...args} />,
+ args: {
+ ...defaultProps,
+ className: "max-w-3xl",
+ },
+};Also applies to: 1159-1194
vinroger
changed the title
Will do. Thank you for reminding!
I am following Junior's edit on autocomplete props component. https://github.com/nextui-org/nextui/pull/4094/files#diff-f28f2caca204e466c560d07fd4d4ca22c4618f1430ea13b915548db84cf7aad8 My thoughts: this is because even if the user does not provide the isVirtualized prop, we enable virtualization if the list item is more than 50. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.changeset/fresh-windows-check.md (1)
1-5: Enhance the changeset description with more details.While the version bump is appropriate, the description could be more comprehensive to better document this significant feature. Consider expanding it to include:
- Key benefits (improved performance for large datasets)
- New props added (isVirtualized, rowHeight, maxTableHeight)
- Mention of backward compatibility
Here's a suggested enhancement:
--- "@nextui-org/table": patch --- -Virtualization support added to Table component +Added virtualization support to Table component: +- Improves performance for large datasets by rendering only visible rows +- New props: isVirtualized, rowHeight (default: 40px), maxTableHeight (default: 600px) +- Maintains backward compatibility with existing implementations +- Powered by @tanstack/react-virtual
Results: Demo for 10000 rows + Full Page Refresh (Delay reduced significantly)
table.virtualized.mp4
📝 Description
This PR introduces virtualization support for the
Tablecomponent, enabling efficient rendering and handling of large datasets by only rendering visible items in the viewport. The implementation utilizes the@tanstack/react-virtuallibrary and adds several customizable props to enhance usability and performance.⛳️ Current behavior (updates)
The current
Tablecomponent renders all rows in the DOM, which can lead to significant performance issues when displaying large datasets.🚀 New behavior
Virtualization Support:
isVirtualizedprop to enable virtualization.rowHeight: Specifies the height of each row (default: 40px).maxTableHeight: Sets the maximum height of the table (default: 600px).Documentation and Examples:
rowHeightfor variable row sizes.maxTableHeightconfigurations.Performance Improvements:
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
This feature is powered by the
@tanstack/react-virtuallibrary for efficient list rendering. All new props and behavior changes are backward-compatible, ensuring seamless integration with existing implementations.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tablecomponent, allowing efficient rendering of large datasets.isVirtualized,rowHeight, andmaxTableHeightfor enhanced configurability.Bug Fixes
TableRowGroupcomponent to improve type safety.Documentation
Tablecomponent with new sections on virtualization and updated API details.Chores