Add initialization of new record sorts state and added remove record sorts util #10358
Conversation
- Added useApplyCurrentViewSortsToCurrentRecordSorts - Added useApplyViewSortsToCurrentRecordSorts - Added unit tests - Renamed to isRecordSortDirectionDropdownMenuUnfoldedComponentState
![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 introduces synchronization between view sorts and record sorts, mirroring the existing pattern used for view and record filters synchronization.
- Added new
useApplyViewSortsToCurrentRecordSortsanduseApplyCurrentViewSortsToCurrentRecordSortshooks in/packages/twenty-front/src/modules/views/hooks/to manage sort state synchronization - Added
useRemoveRecordSorthook in/packages/twenty-front/src/modules/object-record/record-sort/hooks/useRemoveRecordSort.tsfor consistent sort removal - Renamed state from
isRecordSortDirectionMenuUnfoldedtoisRecordSortDirectionDropdownMenuUnfoldedfor better clarity - Added comprehensive test coverage in
/packages/twenty-front/src/modules/views/hooks/__tests__/for new sort synchronization hooks
12 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| const hasFoundRecordSortInCurrentRecordSorts = currentRecordSorts.some( | ||
| (existingSort) => existingSort.fieldMetadataId === fieldMetadataId, | ||
| ); | ||
|
|
||
| if (hasFoundRecordSortInCurrentRecordSorts) { |
There was a problem hiding this comment.
style: Redundant check - the findIndex operation on line 27 already performs this check. This creates unnecessary iteration over the array.
| const hasFoundRecordSortInCurrentRecordSorts = currentRecordSorts.some( | |
| (existingSort) => existingSort.fieldMetadataId === fieldMetadataId, | |
| ); | |
| if (hasFoundRecordSortInCurrentRecordSorts) { | |
| const indexOfSortToRemove = currentRecordSorts.findIndex( | |
| (existingSort) => existingSort.fieldMetadataId === fieldMetadataId, | |
| ); | |
| if (indexOfSortToRemove >= 0) { |
| const handleToggleColumnSort = useCallback( | ||
| (fieldMetadataId: string) => { | ||
| async (fieldMetadataId: string) => { |
There was a problem hiding this comment.
logic: Function is now async but callers may not be prepared to handle the Promise return type. Check all usage sites.
| upsertRecordSort(newSort); | ||
|
|
||
| await upsertCombinedViewSort(newSort); |
There was a problem hiding this comment.
logic: No error handling for upsertCombinedViewSort failure. Record sort could be left in inconsistent state if view sort fails.
| const currentSorts = useRecoilComponentValueV2( | ||
| currentRecordSortsComponentState, | ||
| ); |
There was a problem hiding this comment.
style: Verify initial state of currentSorts before applying changes to ensure clean test environment.
| if (!isDefined(mockFieldMetadataItem)) { | ||
| throw new Error('Missing mock field metadata item with type TEXT'); | ||
| } |
There was a problem hiding this comment.
syntax: Error message mentions 'type TEXT' but the field is actually filtered by 'name'
| if (!isDefined(mockFieldMetadataItem)) { | |
| throw new Error('Missing mock field metadata item with type TEXT'); | |
| } | |
| if (!isDefined(mockFieldMetadataItem)) { | |
| throw new Error('Missing mock field metadata item with name "name"'); | |
| } |
| it('should not apply sorts when current view is not found', () => { | ||
| const { result } = renderHook( | ||
| () => { | ||
| const { applyCurrentViewSortsToCurrentRecordSorts } = | ||
| useApplyCurrentViewSortsToCurrentRecordSorts(); | ||
|
|
||
| const currentSorts = useRecoilComponentValueV2( | ||
| currentRecordSortsComponentState, | ||
| ); | ||
|
|
||
| return { | ||
| applyCurrentViewSortsToCurrentRecordSorts, | ||
| currentSorts, | ||
| }; | ||
| }, | ||
| { | ||
| wrapper: getJestMetadataAndApolloMocksAndActionMenuWrapper({ | ||
| apolloMocks: [], | ||
| componentInstanceId: 'instanceId', | ||
| contextStoreCurrentObjectMetadataNameSingular: | ||
| mockObjectMetadataItemNameSingular, | ||
| onInitializeRecoilSnapshot: (snapshot) => { | ||
| snapshot.set( | ||
| contextStoreCurrentViewIdComponentState.atomFamily({ | ||
| instanceId: 'instanceId', | ||
| }), | ||
| mockView.id, | ||
| ); | ||
|
|
||
| snapshot.set(prefetchViewsState, []); | ||
| }, | ||
| }), | ||
| }, | ||
| ); | ||
|
|
||
| act(() => { | ||
| result.current.applyCurrentViewSortsToCurrentRecordSorts(); | ||
| }); | ||
|
|
||
| expect(result.current.currentSorts).toEqual([]); | ||
| }); |
There was a problem hiding this comment.
logic: Test sets view ID but empty prefetch state - could lead to inconsistent state. Consider testing with non-existent view ID instead
|
|
||
| const currentView = useRecoilValue( | ||
| prefetchViewFromViewIdFamilySelector({ | ||
| viewId: currentViewId ?? '', |
There was a problem hiding this comment.
logic: Empty string fallback could cause unnecessary prefetch calls when currentViewId is null
| const applyCurrentViewSortsToCurrentRecordSorts = () => { | ||
| const sortDefinitions = formatFieldMetadataItemsAsSortDefinitions({ | ||
| fields: sortableFieldMetadataItems, | ||
| }); | ||
|
|
||
| if (isDefined(currentView)) { | ||
| setCurrentRecordSorts( | ||
| mapViewSortsToSorts(currentView.viewSorts, sortDefinitions), | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
logic: No cleanup of record sorts when currentView is undefined - could leave stale sorts
| const applyCurrentViewSortsToCurrentRecordSorts = () => { | |
| const sortDefinitions = formatFieldMetadataItemsAsSortDefinitions({ | |
| fields: sortableFieldMetadataItems, | |
| }); | |
| if (isDefined(currentView)) { | |
| setCurrentRecordSorts( | |
| mapViewSortsToSorts(currentView.viewSorts, sortDefinitions), | |
| ); | |
| } | |
| }; | |
| const applyCurrentViewSortsToCurrentRecordSorts = () => { | |
| const sortDefinitions = formatFieldMetadataItemsAsSortDefinitions({ | |
| fields: sortableFieldMetadataItems, | |
| }); | |
| if (isDefined(currentView)) { | |
| setCurrentRecordSorts( | |
| mapViewSortsToSorts(currentView.viewSorts, sortDefinitions), | |
| ); | |
| } else { | |
| setCurrentRecordSorts([]); | |
| } | |
| }; |
This PR adds the same synchronization utils and hooks from view to record sorts, as we did with view and record filters.
The goal is to apply what's in the view sorts only when needed.
Also added tests for those utils and hooks.
We also add useRemoveRecordSorts.