feat: add basic table infinite scroll function #39441
Conversation
WalkthroughThe changes update the TableWidget V2 components to improve infinite scrolling behavior and data handling. The Table component now applies skeleton loading conditionally based on infinite scroll settings and passes a new Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🔇 Additional comments (5)
✨ Finishing Touches
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx (2)
7-7: Nitpick on naming consistency.Good import for
LoadingIndicator, but watch out for the file and folder name "InifiniteScrollBody" – consider renaming it to "InfiniteScrollBody" for consistency with the new code.
24-30: Props destructuring clarity.The addition of
totalRecordsCountis helpful. Make sure all call sites supply the expected prop or handle undefined gracefully. Consider providing a default value to avoid pre-emptive conditionals in downstream logic.app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsx (1)
39-39: Prop rename frompageSizetoitemCount.Renaming to
itemCountmakes the component's responsibility clearer. Ensure all references are updated accordingly in unit tests and parent components.app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx (4)
1-1: Import grouping consideration.The import is fine, but consider grouping React-related imports together for consistency. This is just a style preference, not a requirement.
19-20: Local cache structure.
LoadedRowsCacheis a straightforward approach. Consider limiting growth if an extremely large dataset is loaded or implementing a removal policy for memory constraints.
40-60: useEffect for caching new rows.The logic to capture partial pages and disable further requests is sound. Double-check concurrency handling if multiple calls to
loadMoreoccur quickly.
97-110: loadMoreItems concurrency.
asyncusage is correct. IfloadMoretriggers additional state updates in quick successions, consider debouncing or controlling requests to avoid race conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/client/src/widgets/TableWidgetV2/component/Table.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx(3 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx(1 hunks)app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx(2 hunks)app/client/src/widgets/TableWidgetV2/widget/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (16)
app/client/src/widgets/TableWidgetV2/component/TableBody/index.tsx (1)
65-65: Good addition of optional totalRecordsCount propertyThe optional property will help track the total number of records available for pagination or infinite scrolling scenarios.
app/client/src/widgets/TableWidgetV2/component/VirtualTable.tsx (2)
42-42: Added totalRecordsCount to VirtualTableProps interfaceCorrectly typed as an optional number property to maintain backward compatibility.
84-84: Properly passing totalRecordsCount to TableBody componentEnsures the property is correctly propagated through the component hierarchy.
app/client/src/widgets/TableWidgetV2/component/Table.tsx (2)
451-452: Better UX for loading states with infinite scrollThe skeleton loading state will now only be applied when the table is loading AND infinite scrolling is not enabled. This improves user experience by showing already loaded data during infinite scroll operations.
535-535: Passing totalRecordsCount to VirtualTable componentNecessary for the infinite scroll functionality to work properly across the component hierarchy.
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/index.tsx (3)
9-9: Verify local usage.The hook
useInfiniteVirtualizationis imported correctly. Ensure no references remain to any older infinite-scrolling logic in other parts of the code.
31-34: Integration with the hook.The
useInfiniteVirtualizationusage provides a clear pattern for infinite scrolling. Confirm thatloadMoreFromEvaluationscorrectly triggers data fetches without concurrency issues, as multiple scroll events could overlap.
44-46: Improved itemCount usage and loading indicator.
- Using the new
itemCountandminimumBatchSizeis a neat approach, replacing the previous hardcoded buffer.- Conditionally rendering
<LoadingIndicator>helps guide the user experience.No further concerns here.
Also applies to: 53-53, 56-56, 61-61
app/client/src/widgets/TableWidgetV2/component/TableBody/VirtualList.tsx (1)
46-46: Directly usingitemCount.Removing the custom calculation and using
itemCountdirectly is simpler. This is a cleaner approach that avoids confusion about which value is controlling the number of items.Also applies to: 61-61
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx (7)
4-10: Interface rename and prop addition.The renaming to
UseInfiniteVirtualizationPropsis consistent. The optionaltotalRecordsCountprop is a good addition for large data sets. Just make sure the calling scope has accurate total counts for optimal performance.
12-17: Refined return interface.The updated
UseInfiniteVirtualizationReturnwithcachedRowsand asyncloadMoreItemsis well-defined. Good job ensuring each part is typed properly.
26-29: State and references initialization.
- Using
lastLoadedPageRefandhasMoreDataRefis a nice way to track loading progress.maxPages = Infinityif no total is provided is practical, but beware of unbounded scroll scenarios.Also applies to: 30-32, 34-38
62-73: Memoized cached rows.Building a single array from
loadedPagesensures minimal re-render. Keep an eye on memory usage for extremely large tables.
75-86: isItemLoaded condition.This function effectively prevents redundant loading. Just confirm that partial page loads won't trigger repeated fetch calls after the last loaded page is set.
88-95: itemCount fallback logic.Capping
itemCounttocachedRows.lengthwhen no more data is available is reasonable. Ensure an accurate count is used iftotalRecordsCountupdates asynchronously.
113-117: Exposed return interface.No concerns with this final return object. The pattern is clear and well aligned with the rest of the codebase.
| @@ -1960,7 +1960,7 @@ class TableWidgetV2 extends BaseWidget<TableWidgetProps, WidgetState> { | |||
| originalIndex = rowIndex === 0 ? -1 : row[ORIGINAL_INDEX_KEY] ?? rowIndex; | |||
| } else { | |||
| row = filteredTableData[rowIndex]; | |||
| originalIndex = row[ORIGINAL_INDEX_KEY] ?? rowIndex; | |||
| originalIndex = row ? row[ORIGINAL_INDEX_KEY] ?? rowIndex : rowIndex; | |||
There was a problem hiding this comment.
Added safety check to prevent null reference exceptions
The addition of a null check before accessing the row property prevents potential runtime errors if row is undefined.
There was a problem hiding this comment.
@jacquesikot Can you explain the importance of this logic? It's not clear.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@rahulbarwal There is a situation where row is undefined, when user scrolls, infinite scroll loads empty records into the table, before it populates with actual data. This is to fix an error when row is undefined during the infinite scroll process
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx (2)
39-59: Consider additional safeguards in the useEffect.The effect logic for updating the page cache is generally sound, but consider additional safeguards for edge cases, such as handling out-of-order page loads that could occur in certain network conditions.
useEffect(() => { if (rows.length > 0) { const currentPageIndex = lastLoadedPageRef.current; + // Ensure we're not overwriting newer data with older data + if (loadedPages[currentPageIndex] && loadedPages[currentPageIndex].length > 0) { + // If this page is already loaded, we might be receiving duplicate data + console.debug(`Page ${currentPageIndex} already loaded, potential race condition`); + } setLoadedPages((prev) => ({ ...prev, [currentPageIndex]: rows, })); // Only increment if we got a full page or some data if (rows.length === pageSize) { lastLoadedPageRef.current = currentPageIndex + 1; } else if (rows.length < pageSize && rows.length > 0) { // If we got less than a full page, assume this is the last page hasMoreDataRef.current = false; } } else if (rows.length === 0 && lastLoadedPageRef.current > 0) { // If no rows are returned and we've loaded at least one page, assume end of data hasMoreDataRef.current = false; } }, [rows, pageSize]);
96-109: Consider making loadMore actually asynchronous.The function returns a Promise but doesn't await
loadMore(). IfloadMoreis or becomes asynchronous in the future, this could lead to race conditions.const loadMoreItems = useCallback( async (startIndex: number, stopIndex: number) => { if (!isLoading && hasMoreDataRef.current) { const targetPage = Math.floor(stopIndex / pageSize); if (targetPage >= lastLoadedPageRef.current && targetPage < maxPages) { - loadMore(); + try { + await Promise.resolve(loadMore()); + } catch (error) { + console.error("Error loading more items:", error); + } } } return Promise.resolve(); }, [isLoading, loadMore, pageSize, maxPages], );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.test.tsx(4 hunks)app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
🔇 Additional comments (10)
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.test.tsx (5)
7-37: Good addition of a mock factory function for test data.The
createMockRowsfunction is well-designed for generating test data dynamically. This approach makes tests more maintainable and flexible compared to hardcoded mock data.
72-78: Updated test case reflects new hook behavior.This test correctly validates that the
cachedRowsstate is properly initialized with the provided rows.
80-94: Improved test structure with explicit mock declarations.Moving the
loadMoremock into the test scope provides better isolation and makes the test's dependencies more explicit.
113-143: Clearer test cases with descriptive comments.The updated test cases with comments explaining what's being tested (e.g., "Index within loaded range") improve readability and maintainability.
156-315: Comprehensive test coverage for pagination scenarios.The additional test cases thoroughly cover important edge cases:
- Caching multiple page loads
- Handling partial page loads
- Detecting end of data
- Handling empty page loads
- Respecting maxPages limits
- Maintaining page references during rerenders
This robust test suite will help prevent regressions in the infinite scroll functionality.
app/client/src/widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx (5)
3-9: Appropriate interface renaming for clarity.Renaming to
UseInfiniteVirtualizationPropsfollows standard hook naming conventions and better reflects the interface's purpose.
11-16: Enhanced return interface with necessary properties.Adding
cachedRowsto the return interface is essential for components that need access to all loaded rows across pagination.
29-38: Well-structured state management for pagination.The state management approach using
useStatefor the cache and refs for tracking pagination state is efficient. ThemaxPagescalculation withuseMemois also appropriate.
61-72: Efficient caching implementation with ordered page handling.The implementation for
cachedRowscorrectly sorts and combines pages in the right order, which is crucial for maintaining row sequence in the UI.
74-95: Optimized item loading detection and count calculation.The implementations of
isItemLoadedanditemCountare well-optimized, using appropriate memoization and considering the available data state.
github-actions
bot
added
Query & Widgets Pod
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13541322404. |
|
Deploy-Preview-URL: https://ce-39441.dp.appsmith.com |
.../widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx
Show resolved
Hide resolved
.../widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx
Show resolved
Hide resolved
| @@ -1960,7 +1960,7 @@ class TableWidgetV2 extends BaseWidget<TableWidgetProps, WidgetState> { | |||
| originalIndex = rowIndex === 0 ? -1 : row[ORIGINAL_INDEX_KEY] ?? rowIndex; | |||
| } else { | |||
| row = filteredTableData[rowIndex]; | |||
| originalIndex = row[ORIGINAL_INDEX_KEY] ?? rowIndex; | |||
| originalIndex = row ? row[ORIGINAL_INDEX_KEY] ?? rowIndex : rowIndex; | |||
There was a problem hiding this comment.
@jacquesikot Can you explain the importance of this logic? It's not clear.
.../widgets/TableWidgetV2/component/TableBody/InifiniteScrollBody/useInfiniteVirtualization.tsx
Show resolved
Hide resolved
jacquesikot
added
ok-to-test
## Description Empty rows were not getting rendered for the server side pagination enabled due to incorrect calculation of itemCount(introduced here: #39441). | Previously | Now | |----------|----------| | https://github.com/user-attachments/assets/5f0db0e9-294f-461a-bac6-d3b2fe573817 | https://github.com/user-attachments/assets/e8678e15-68e5-4f9e-8b2e-b74cf9838a9f | Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Table" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/13722106486> > Commit: ebbe90b > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=13722106486&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Table` > Spec: > <hr>Fri, 07 Mar 2025 14:27:47 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Revised the table widget’s row rendering so that it now displays the full expected number of rows based on the selected page size, ensuring consistent and reliable pagination behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Infinite Scroll Improvements for TableWidgetV2
This PR enhances the infinite scroll functionality in
TableWidgetV2with several key improvements:🚀 Major Changes
1️⃣ Improved Loading State Management
shouldShowSkeleton()function to determine when to display loading skeletons.2️⃣ Enhanced Infinite Scroll Implementation
useInfiniteVirtualizationhook to properly cache and manage loaded rows.3️⃣ Better Virtualization Support
itemCount.totalRecordsCountto optimize infinite scroll behavior.🧪 Testing
This PR includes comprehensive test coverage for the infinite scroll functionality, with 12 detailed test cases covering:
Fixes #39083
Automation
/ok-to-test tags="@tag.Binding, @tag.Sanity, @tag.Table, @tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13651510022
Commit: 3bbfda7
Cypress dashboard.
Tags:
@tag.Binding, @tag.Sanity, @tag.Table, @tag.WidgetSpec:
Tue, 04 Mar 2025 11:47:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Tablecomponent's rendering logic.