-
Notifications
You must be signed in to change notification settings - Fork 4.3k
🦣🦣🦣 Table virtualization #14743
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
base: main
Are you sure you want to change the base?
🦣🦣🦣 Table virtualization #14743
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.
Greptile Overview
Summary
This PR implements table virtualization for the record table to improve performance when handling large datasets. The implementation introduces a virtualized row system that renders only visible rows plus a buffer, dynamically fetches data as users scroll, and manages state through a treadmill pattern that reuses DOM elements.
Key Changes
- Virtualization Core:
RecordTableRowVirtualized
component that renders rows based on virtual/real index mapping - Data Loading:
RecordTableVirtualizedDataLoaderEffect
handles progressive data fetching with batched UI updates - Scroll Management:
RecordTableVirtualizedRowTreadmillEffect
manages the scroll-to-virtualization mapping with proper debouncing - State Management: Multiple new state atoms track virtualization indices, fetch status, and scroll positions
- Utility Function: New
getRange
utility added totwenty-shared
for array operations
Issues Found
- Debug output present: The virtualized row component contains debug output that should be removed before production
- Missing error handling: Async data loading operations lack proper error handling which could cause silent failures
- Hardcoded values: Magic numbers used instead of established constants for consistency
- API design:
getRange
function has confusing parameter semantics that could lead to off-by-one errors
Confidence Score: 3/5
- This PR introduces significant performance improvements but contains debug output and missing error handling that need attention
- Score reflects the architectural complexity and solid implementation approach, but several issues must be addressed: debug output needs removal, async operations need error handling, and hardcoded values should use constants. The virtualization logic itself is well-structured with proper debouncing and state management.
- Pay close attention to RecordTableRowVirtualized.tsx (debug output removal) and RecordTableVirtualizedDataLoaderEffect.tsx (error handling)
Important Files Changed
File Analysis
Filename | Score | Overview |
---|---|---|
packages/twenty-front/src/modules/object-record/record-table/virtualization/components/RecordTableRowVirtualized.tsx | 2/5 | Core virtualized row component with debug output that needs removal and hardcoded values that should use constants |
packages/twenty-front/src/modules/object-record/record-table/virtualization/components/RecordTableVirtualizedDataLoaderEffect.tsx | 3/5 | Data loading logic with missing error handling for async operations that could cause silent failures |
packages/twenty-shared/src/utils/array/getRange.ts | 3/5 | Utility function with confusing parameter naming that could lead to off-by-one errors |
Sequence Diagram
sequenceDiagram
participant User
participant RecordTable
participant VirtualBody
participant DataLoader
participant TreadmillEffect
participant API
User->>RecordTable: Initial load
RecordTable->>VirtualBody: Mount virtualized body
VirtualBody->>DataLoader: Initialize data loading
DataLoader->>API: fetchRecordsLazy()
API-->>DataLoader: records + totalCount
DataLoader->>VirtualBody: resetVirtualizedRowTreadmill()
DataLoader->>VirtualBody: setRecordTableData(records)
User->>RecordTable: Scroll down
RecordTable->>TreadmillEffect: handleScroll event
TreadmillEffect->>TreadmillEffect: Calculate new visible range
TreadmillEffect->>VirtualBody: Update realIndexByVirtualIndex mapping
Note over TreadmillEffect: Checks if more data needed
alt lastRealIndexSet > hasAlreadyFetchedUpToRealIndex
TreadmillEffect->>DataLoader: Trigger fetch more
DataLoader->>API: fetchMoreRecords(150)
API-->>DataLoader: more records
DataLoader->>DataLoader: Batch process records (10 at a time)
loop For each batch
DataLoader->>VirtualBody: Update recordIdByRealIndex state
DataLoader->>DataLoader: Wait 50ms between batches
end
end
VirtualBody->>RecordTable: Render virtualized rows
RecordTable-->>User: Display updated table
46 files reviewed, 7 comments
...c/modules/object-record/record-table/virtualization/components/RecordTableRowVirtualized.tsx
Outdated
Show resolved
Hide resolved
...c/modules/object-record/record-table/virtualization/components/RecordTableRowVirtualized.tsx
Outdated
Show resolved
Hide resolved
...c/modules/object-record/record-table/virtualization/components/RecordTableRowVirtualized.tsx
Outdated
Show resolved
Hide resolved
...c/modules/object-record/record-table/virtualization/components/RecordTableRowVirtualized.tsx
Outdated
Show resolved
Hide resolved
...ect-record/record-table/virtualization/components/RecordTableVirtualizedDataLoaderEffect.tsx
Outdated
Show resolved
Hide resolved
export const getRange = (start: number, end: number) => { | ||
return Array.from({ length: end }, (_, index) => index + start) | ||
} |
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.
logic: Function behavior is confusing - getRange(0, 300)
generates 0-299, not 0-300 as might be expected
export const getRange = (start: number, end: number) => { | |
return Array.from({ length: end }, (_, index) => index + start) | |
} | |
export const getRange = (start: number, length: number) => { | |
return Array.from({ length }, (_, index) => index + start); | |
}; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-shared/src/utils/array/getRange.ts
Line: 1:3
Comment:
logic: Function behavior is confusing - `getRange(0, 300)` generates 0-299, not 0-300 as might be expected
```suggestion
export const getRange = (start: number, length: number) => {
return Array.from({ length }, (_, index) => index + start);
};
```
How can I resolve this? If you propose a fix, please make it concise.
totalNumberOfRecordsToVirtualize, | ||
]); | ||
|
||
return <></>; |
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.
style: Use null
instead of empty fragment when returning nothing
return <></>; | |
return null; |
Context Used: Context - Avoid using fragments when there is only one child component in a return statement. (link)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/object-record/record-table/virtualization/components/RecordTableNoRecordGroupVirtualizedBodyEffect.tsx
Line: 108:108
Comment:
style: Use `null` instead of empty fragment when returning nothing
```suggestion
return null;
```
**Context Used:** **Context -** Avoid using fragments when there is only one child component in a return statement. ([link](https://app.greptile.com/review/custom-context?memory=51414064-3127-4b1d-ad7e-62ce2c3739e9))
How can I resolve this? If you propose a fix, please make it concise.
...ect-record/record-table/virtualization/components/RecordTableVirtualizedDataLoaderEffect.tsx
Outdated
Show resolved
Hide resolved
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:43433 This environment will automatically shut down when the PR is closed or after 5 hours. |
bbb5d12
to
8b625bf
Compare
2bcf465
to
4e97a8f
Compare
2e7760d
to
c04d362
Compare
c04d362
to
a1c0971
Compare
ac6eaae
to
1ea2596
Compare
}); | ||
|
||
if (dragOperationType === 'single') { | ||
const targetRecordId = allSparseRecordIds.at( | ||
result.destination.index, | ||
); | ||
|
||
if (!isDefined(targetRecordId)) { | ||
throw new Error( | ||
`Target record id cannot be found, this should not happen`, | ||
); | ||
} | ||
|
||
const singleDragResult = processSingleDrag({ | ||
result, | ||
recordPositionData, | ||
recordIds: allRecordIds, | ||
sourceRecordId: draggedRecordId, | ||
targetRecordId: targetRecordId ?? '', | ||
recordsWithPosition: contiguousRecordsWithPosition, | ||
}); |
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.
Potential bug: A record's position
can be null
, but it's passed to a function expecting a number
, causing a runtime error during drag-and-drop.
-
Description: During a drag-and-drop operation, records are mapped to an array of objects with an
id
andposition
. Theposition
is derived usinggetSnapshotValue(...)?.position ?? null
, which can result innull
if a record's data is not fully loaded in the store. This array is then passed tosortByProperty
, which does not supportnull
values and will throw an error:"Property type not supported in sortByProperty, only string and number are supported"
. This will cause the drag-and-drop feature to fail. -
Suggested fix: Modify the position retrieval logic to provide a default numeric value instead of
null
. For example, change?.position ?? null
to?.position ?? 0
, aligning with theposition
field's default value in the entity definition.
severity: 0.8, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
activityRecord: Note | Task; | ||
objectMetadataItems: ObjectMetadataItem[]; | ||
activityTargets?: NoteTarget[] | TaskTarget[]; | ||
activityTargets?: Nullable<NoteTarget[] | TaskTarget[]>; |
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.
ok :)
$orderBy: [MessageOrderByInput] | ||
$lastCursor: String | ||
$limit: Int | ||
$offset: Int |
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.
why this change? looks unrelated to the PR
'objectNameSingular' | ||
>; | ||
|
||
export const useLazyFindManyRecordsWithOffset = ({ |
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.
fileName <> componentName
border-left: 1px solid transparent; | ||
`; | ||
|
||
export const RecordTableCellCheckboxPlaceholder = () => { |
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.
would be nice to uniformize naming Skeleton <> Placeholder
realIndex: number; | ||
}; | ||
|
||
export const RecordTableRowVirtualizedRouterLevel1 = ({ |
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.
Router naming is confusing!
lastObjectOperation?.id !== | ||
lastObjectOperationThatResettedVirtualization?.id | ||
) { | ||
setLastObjectOperationThatResettedVirtualization(lastObjectOperation); |
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.
I think we should discuss this, this seems hard to maintain
import { useEffect } from 'react'; | ||
|
||
// TODO: see if we can merge the initial and load more processes, to have only one load at scroll index effect | ||
export const RecordTableVirtualizedInitialDataLoadEffect = () => { |
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.
that's a lot of effects would be great to see if we can refactor. DevXP is a bit complex regarding data loading, we might want to take a tradeoff to simplify
.getLoadable(recordStoreFamilyState(record.id)) | ||
.getValue(); | ||
|
||
if (JSON.stringify(currentRecord) !== JSON.stringify(record)) { |
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.
isDeeplyEqual
This big PR implements table virtualization with an offset paging, allowing a way more fluid UX.
It is a v1 that should be improved in the future with partial data loading and optimization of the browser display performance of a row.
But with this PR we have the solid enough technical foundation, both frontend and backend, to get to a smooth table UX.
Fixes and improvements after first successful round of development (needed to have main clean) :
Fixes twentyhq/core-team-issues#1613 that contains other bugs to be fixed before merge