Removed sort definitions #10368
Removed sort definitions #10368
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR removes sort definitions across the codebase, simplifying the sorting system by eliminating the intermediate SortDefinition type and related utilities.
- Removed
SortDefinitiontype and itsdefinitionproperty fromRecordSort, now relying directly on field metadata for sorting information - Replaced
formatFieldMetadataItemsAsSortDefinitionswithavailableFieldMetadataItemsForSortFamilySelectorfor handling sortable fields - Removed
availableSortDefinitionsComponentStateand related Recoil states, simplifying state management for sorting - Modified
mapViewSortsToSortsto work without sort definitions, potentially allowing invalid field metadata IDs to pass through - Removed mock sort definitions from sign-in background feature, suggesting a shift to more dynamic sorting approaches
28 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
logic: Removing this state could break components that depend on availableSortDefinitionsComponentState, particularly ViewBarSortEffect, ObjectSortDropdownButton, and record index components. Ensure all dependent components are updated to handle sorting without this state.
lucasbordeau
added
-PR: awaiting review
and removed
-PR: wip
-PR: awaiting author
labels
There was a problem hiding this comment.
PR Summary
(updates since last review)
Based on the additional changes since the last review, here's a focused summary of the key updates:
This PR continues the removal of sort definitions with additional cleanup and refinements across the codebase.
- Missing ID generation in
useUpsertCombinedViewSorts.tscould cause undefined behavior when matching/updating sorts without unique IDs - Removed
useSortableFieldMetadataItemsInRecordIndexContexthook without clear replacement, impacting record index sorting functionality - Updated
EditableSortChipto usefieldMetadataItem.labeldirectly instead ofrecordSort.definition.label - Removed position-based sorting in
useSetRecordIdsForColumn, which could affect record ordering in columns - Simplified
ViewBarDetailsto usecurrentRecordSortsdirectly rather than mapping view sorts
The changes maintain the goal of removing sort definitions but introduce some potential issues around ID generation and record ordering that should be addressed.
31 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| const filteredSearchInputFieldMetadataItems = | ||
| sortableFieldMetadataItems.filter((item) => | ||
| item.label | ||
| .toLocaleLowerCase() | ||
| .includes(objectSortDropdownSearchInput.toLocaleLowerCase()), | ||
| ); | ||
| ); |
There was a problem hiding this comment.
style: Consider memoizing this filter operation to prevent unnecessary recalculations on each render
There was a problem hiding this comment.
logic: Removing this utility will break sorting in ObjectSortDropdownButton, useColumnDefinitionsFromFieldMetadata, and view management. Need replacement implementation or rollback.
There was a problem hiding this comment.
logic: Removing SortDefinition type will break existing sort functionality in ObjectSortDropdownButton, EditableSortChip, and other components that rely on this type. Ensure all consuming components are updated to use the new sorting mechanism.
There was a problem hiding this comment.
logic: This hook is used by ObjectSortDropdownButton.tsx and other components for determining which fields can be sorted. Ensure all consumers have been updated to use an alternative method for accessing sortable fields.
There was a problem hiding this comment.
style: Consider keeping this hook and deprecating it first to allow for a smoother transition.
|
I have tested and it works fine EXCEPT on Group By views |
This PR focuses on complete removal of sort definitions.