Fix scroll to start when resize or move around columns#15655
Conversation
|
|
||
| if (shouldRefetchData) { | ||
| await triggerInitialRecordTableDataLoad({ | ||
| shouldScrollToStart: isEmpty(lastFields), |
There was a problem hiding this comment.
isEmpty(lastFields) helps determining if it is an initial load or not
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Added conditional scroll behavior to prevent table from scrolling to the start when columns are resized or moved. The fix introduces a shouldScrollToStart parameter that only scrolls when there were no previous fields loaded.
Key changes:
- Only refetches data when column count increases (
currentFields.length > lastFields.length) - Only scrolls to start position when transitioning from empty to non-empty field list (
isEmpty(lastFields)) - Always resets table focus state regardless of scroll behavior
Issue found:
- The
shouldRefetchDatalogic only checks if columns were added (>), not if they were removed or reordered. This means column removals and reorderings won't trigger a refetch, which could lead to data display issues.
Confidence Score: 3/5
- This PR partially fixes the scroll issue but introduces a logic bug that prevents proper handling of column removal and reordering
- The fix addresses the reported scroll-to-start issue when columns are resized, but the
shouldRefetchDatalogic only handles column additions (>), not removals or reorderings. This creates scenarios where the table won't refetch data when columns are removed or reordered, potentially causing display inconsistencies. - RecordTableVirtualizedInitialDataLoadEffect.tsx needs correction to the shouldRefetchData condition on line 82
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-front/src/modules/object-record/record-table/virtualization/components/RecordTableVirtualizedInitialDataLoadEffect.tsx | 3/5 | Added conditional logic to prevent unnecessary data refetches and scrolling when only column order changes. Only refetches when columns are added, and only scrolls to start when there were no previous fields. |
Sequence Diagram
sequenceDiagram
participant User
participant Effect as RecordTableVirtualizedInitialDataLoadEffect
participant Hook as useTriggerInitialRecordTableDataLoad
participant Table as Record Table
User->>Table: Resize or move column
Table->>Effect: visibleRecordFields changes
Effect->>Effect: Compare lastFields vs currentFields
alt currentFields.length > lastFields.length
Effect->>Effect: shouldRefetchData = true
alt isEmpty(lastFields)
Effect->>Hook: triggerInitialRecordTableDataLoad({shouldScrollToStart: true})
Hook->>Hook: resetTableFocuses()
Hook->>Hook: Load data
Hook->>Table: scrollTableToPosition({0, 0})
else lastFields not empty
Effect->>Hook: triggerInitialRecordTableDataLoad({shouldScrollToStart: false})
Hook->>Hook: resetTableFocuses()
Hook->>Hook: Load data
Note over Hook,Table: No scroll - preserves position
end
else currentFields.length <= lastFields.length
Note over Effect: shouldRefetchData = false<br/>No refetch, no scroll
end
2 files reviewed, 1 comment
| const lastFields = lastContextStoreVisibleRecordFields || []; | ||
| const currentFields = visibleRecordFields || []; | ||
|
|
||
| const shouldRefetchData = currentFields.length > lastFields.length; |
There was a problem hiding this comment.
logic: this logic will not handle column removal or reordering correctly. When columns are removed, currentFields.length will be less than or equal to lastFields.length, so shouldRefetchData will be false and data won't be refetched. Column reordering would also be skipped since lengths are equal.
| const shouldRefetchData = currentFields.length > lastFields.length; | |
| const shouldRefetchData = currentFields.length !== lastFields.length; |
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/RecordTableVirtualizedInitialDataLoadEffect.tsx
Line: 82:82
Comment:
**logic:** this logic will not handle column removal or reordering correctly. When columns are removed, `currentFields.length` will be less than or equal to `lastFields.length`, so `shouldRefetchData` will be false and data won't be refetched. Column reordering would also be skipped since lengths are equal.
```suggestion
const shouldRefetchData = currentFields.length !== lastFields.length;
```
How can I resolve this? If you propose a fix, please make it concise.
...ecord/record-table/virtualization/components/RecordTableVirtualizedInitialDataLoadEffect.tsx
Show resolved
Hide resolved
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:21743 This environment will automatically shut down when the PR is closed or after 5 hours. |
Fixes https://github.com/twentyhq/private-issues/issues/361
There's room for more improvement here - triggerInitialRecordTableDataLoad does a lot of things, should not be triggered so much. actually even RecordTableVirtualizedInitialDataLoadEffect should not be triggered when there's just a metadata field change (if we trust the name initialDataLoad)