-
Notifications
You must be signed in to change notification settings - Fork 14
fix: scrolling performance optimizations for table #2335
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR aims to optimize scrolling performance for the table component by refining the way visible and fetchable chunks are determined. Key changes include replacing the overscanCount with separate render and fetch overscan parameters, refactoring the chunk state calculation logic, and updating the TableChunk and PaginatedTable components accordingly.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/components/PaginatedTable/useScrollBasedChunks.ts | Reworked overscan logic with new renderOverscan, fetchOverscan, and tableOffset parameters, and updated chunk state calculations. |
src/components/PaginatedTable/TableChunk.tsx | Replaced the isActive prop with shouldRender and shouldFetch, updating skip conditions and display settings accordingly. |
src/components/PaginatedTable/PaginatedTable.tsx | Updated component usage to work with the new chunk state structure and added tableOffset calculation in a layout effect. |
Comments suppressed due to low confidence (3)
src/components/PaginatedTable/useScrollBasedChunks.ts:51
- The tableOffset prop is now required in the hook’s parameters but is not used in the calculateVisibleRange function. If tableOffset is intended to adjust the visible area calculation, consider incorporating it; otherwise, remove this parameter to avoid confusion.
const tableOffset = calculateElementOffsetTop(table, container);
src/components/PaginatedTable/PaginatedTable.tsx:105
- [nitpick] Using scrollContainerRef.current and tableRef.current directly in the dependency array might cause unnecessary re-renders. Consider refactoring to use stable references or ensure that these dependencies are managed correctly to improve maintainability.
React.useLayoutEffect(() => { ... }, [scrollContainerRef.current, tableRef.current, foundEntities]);
src/components/PaginatedTable/TableChunk.tsx:90
- [nitpick] The updated check using shouldFetch instead of the old isActive prop is correct; please ensure that the change in logic maintains the intended debounce behavior across all scenarios.
if (shouldFetch && isTimeoutActive) {
4 rows are on my display. When I move scroll, 8 data request are triggered. It seems it's too much...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves scrolling performance for the paginated table by batching fetch requests, refining chunk rendering logic, and optimizing scroll event handling.
- Integrates a
requestBatcher
to merge consecutive data fetch calls in the Redux reducer. - Refactors
useScrollBasedChunks
to separate render vs. fetch overscan, use throttling, and support Safari-specific parameters. - Extracts chunk rendering into
TableChunksRenderer
and updates row components to apply explicit height styling.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/store/reducers/tableData.ts | Wired the new requestBatcher into the fetchTableChunk endpoint. |
src/components/PaginatedTable/useScrollBasedChunks.ts | Replaced rafThrottle with lodash.throttle , added separate render/fetch overscan, and passive scroll listener. |
src/components/PaginatedTable/requestBatcher.ts | Added a standalone batching utility for merging paginated requests. |
src/components/PaginatedTable/TableRow.tsx | Applied explicit height style to loading, data, and empty rows. |
src/components/PaginatedTable/TableChunksRenderer.tsx | New component to orchestrate chunk separators and rendering based on shouldFetch /shouldRender . |
src/components/PaginatedTable/TableChunk.tsx | Simplified to return row fragments controlled by shouldRender and shouldFetch . |
src/components/PaginatedTable/PaginatedTable.tsx | Replaced inline chunk logic with TableChunksRenderer and computed tableOffset . |
Comments suppressed due to low confidence (4)
src/components/PaginatedTable/TableChunksRenderer.tsx:153
- [nitpick] Fix the typo in the comment: 'distrubited' should be 'distributed'.
// Chunk states are distrubited like [null, null, fetch, fetch, render+fetch, render+fetch, fetch, fetch, null, null]
src/components/PaginatedTable/useScrollBasedChunks.ts:108
- The scroll listener was added with
{ passive: true }
but removed without matching options; use the same options inremoveEventListener
to ensure the handler is properly detached.
container.removeEventListener('scroll', throttledHandleScroll);
src/components/PaginatedTable/requestBatcher.ts:28
- [nitpick] New batching logic in
RequestBatcher
is substantial and critical; consider adding unit tests to cover group formation, batch execution, and error handling.
class RequestBatcher {
src/components/PaginatedTable/requestBatcher.ts:33
- The
queueRequest
method accepts anAbortSignal
but does not handlesignal.aborted
to reject pending promises; add an abort listener to properly cancel and reject on abort.
queueRequest<T, F>(
@Raubzeug actually think I'm done here |
Closes #2193
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: 🔺
Current: 83.65 MB | Main: 83.62 MB
Diff: +0.02 MB (0.03%)
ℹ️ CI Information