Added parallel code path to set new record sorts state #10345
Conversation
- Implemented ViewBarRecordSortEffect - Added availableFieldMetadataItemsForSortFamilySelector - Renamed sort to recordSort where possible
![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 implements a parallel state management system for record sorting, focusing on separating view sorts from record sorts for better state control. Here's a summary of the key changes:
- Added
availableFieldMetadataItemsForSortFamilySelectorin/packages/twenty-front/src/modules/object-metadata/states/availableFieldMetadataItemsForSortFamilySelector.tsto manage sortable field metadata - Introduced
ViewBarRecordSortEffectin/packages/twenty-front/src/modules/views/components/ViewBarRecordSortEffect.tsxto handle sort initialization and state synchronization - Replaced
Sorttype withRecordSorttype across multiple components for better type safety and consistency - Added
useUpsertRecordSorthook in/packages/twenty-front/src/modules/object-record/record-sort/hooks/useUpsertRecordSort.tsfor managing record sort state updates - Implemented
filterSortableFieldMetadataItemsin/packages/twenty-front/src/modules/object-metadata/utils/filterSortableFieldMetadataItems.tsto determine which fields can be sorted
31 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| {RECORD_SORT_DIRECTIONS.map((sortDirection, index) => ( | ||
| <MenuItem | ||
| key={index} | ||
| onClick={() => handleSortDirectionClick(sortDirection)} |
There was a problem hiding this comment.
style: Using array index as key prop may cause issues with React reconciliation if items are reordered. Consider using sortDirection as the key instead
| {RECORD_SORT_DIRECTIONS.map((sortDirection, index) => ( | |
| <MenuItem | |
| key={index} | |
| onClick={() => handleSortDirectionClick(sortDirection)} | |
| {RECORD_SORT_DIRECTIONS.map((sortDirection) => ( | |
| <MenuItem | |
| key={sortDirection} | |
| onClick={() => handleSortDirectionClick(sortDirection)} |
|
|
||
| export const onSortSelectComponentState = createComponentStateV2< | ||
| ((sort: Sort) => void) | undefined | ||
| ((sort: RecordSort) => void) | undefined | ||
| >({ | ||
| key: 'onSortSelectComponentState', |
There was a problem hiding this comment.
style: Consider adding a more descriptive key that includes 'record' to match the new type (e.g., 'onRecordSortSelectComponentState')
| const foundRecordSortInCurrentRecordSorts = currentRecordSorts.some( | ||
| (existingSort) => | ||
| existingSort.fieldMetadataId === recordSortToSet.fieldMetadataId, | ||
| ); |
There was a problem hiding this comment.
logic: foundRecordSortInCurrentRecordSorts is a boolean but the isDefined check on line 26 is redundant since .some() always returns a boolean
| const foundRecordSortInCurrentRecordSorts = currentRecordSorts.some( | |
| (existingSort) => | |
| existingSort.fieldMetadataId === recordSortToSet.fieldMetadataId, | |
| ); | |
| const foundRecordSortInCurrentRecordSorts = currentRecordSorts.some( | |
| (existingSort) => | |
| existingSort.fieldMetadataId === recordSortToSet.fieldMetadataId, | |
| ); | |
| if (!foundRecordSortInCurrentRecordSorts) { |
| const indexOfSortToUpdate = newCurrentRecordSorts.findIndex( | ||
| (existingSort) => | ||
| existingSort.fieldMetadataId === | ||
| recordSortToSet.fieldMetadataId, | ||
| ); |
There was a problem hiding this comment.
logic: no bounds check before array access on line 41 - findIndex could return -1 if no match is found
| const indexOfSortToUpdate = newCurrentRecordSorts.findIndex( | |
| (existingSort) => | |
| existingSort.fieldMetadataId === | |
| recordSortToSet.fieldMetadataId, | |
| ); | |
| const indexOfSortToUpdate = newCurrentRecordSorts.findIndex( | |
| (existingSort) => | |
| existingSort.fieldMetadataId === | |
| recordSortToSet.fieldMetadataId, | |
| ); | |
| if (indexOfSortToUpdate === -1) { | |
| return newCurrentRecordSorts; | |
| } |
| fields: sortableFieldMetadataItems, | ||
| }); | ||
|
|
||
| if (isDefined(currentView)) { |
There was a problem hiding this comment.
logic: Redundant isDefined check since it's already checked on line 54
| if (isDefined(currentView)) { | |
| const sortDefinitions = formatFieldMetadataItemsAsSortDefinitions({ | |
| fields: sortableFieldMetadataItems, | |
| }); | |
| setCurrentRecordSorts( | |
| mapViewSortsToSorts(currentView.viewSorts, sortDefinitions), | |
| ); | |
| setHasInitializedCurrentRecordSorts(true); |
| }); | ||
| export const isRecordSortDirectionMenuUnfoldedComponentState = | ||
| createComponentStateV2<boolean>({ | ||
| key: 'isRecordSortDirectionMenuUnfoldedComponentState', |
There was a problem hiding this comment.
suggestion: i'm just wondering if the naming should be even more "scoped": isRecordSortDirectionDropdownMenuUnfoldedComponentState
This PR implements a parallel code path to set record sorts without impacting the actual sort, like we did on record filter.
We add a ViewBarRecordSortEffect which mirrors ViewBarRecordFilterEffect.
It also adds availableFieldMetadataItemsForSortFamilySelector to replace sortDefinitions progressively.