Implemented CRUD for view filter group and removed old states #10590
Conversation
- Removed useDeleteCombinedViewFilterGroup - Removed useUpsertCombinedViewFilterGroup - Removed useResetUnsavedViewStates - Removed useGetViewFilterGroupsCombined - Removed unsavedToDeleteViewFilterGroupIdsComponentFamilyState - Removed unsavedToUpsertViewFilterGroupsComponentFamilyState - Replaced useGetCurrentView by useGetCurrentViewOnly everywhere viewFilterGroup where used - Renamed viewFilterGroup by recordFilterGroup everywhere view isn't relevant
![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 refactors the advanced filter module by replacing view filter groups with record filter groups, creating a more consistent approach to filter management.
- Renamed properties from
viewFilterGroupIdtorecordFilterGroupIdandpositionInViewFilterGrouptopositionInRecordFilterGroupacross multiple components - Removed
useUpsertCombinedViewFilterGroupanduseDeleteCombinedViewFilterGrouphooks in favor of direct record filter operations - Added new
useSaveRecordFilterGroupsToViewFilterGroupshook to handle saving record filter groups to view filter groups - Replaced
useGetCurrentViewwithuseGetCurrentViewOnlyto simplify view access without combined filters and sorts - Removed old Recoil state management files like
unsavedToDeleteViewFilterGroupIdsComponentFamilyStateandunsavedToUpsertViewFilterGroupsComponentFamilyState
54 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| recordFilterGroupId: newRecordFilterGroupId, | ||
| positionInRecordFilterGroup: newPositionInRecordFilterGroup, | ||
| label: defaultFieldMetadataItem.label, |
There was a problem hiding this comment.
logic: positionInRecordFilterGroup is reused here with the same value as the parent filter group position. This might cause positioning issues if multiple filters are added to the same group.
|
|
||
| return { | ||
| currentViewFilterGroup: viewFilterGroup, | ||
| currentViewFilterGroup: currentRecordFilterGroup, |
There was a problem hiding this comment.
logic: Returning currentRecordFilterGroup as currentViewFilterGroup creates type inconsistency
| @@ -156,16 +156,14 @@ export const ObjectFilterDropdownFilterSelect = ({ | |||
| visibleColumnsFieldMetadataItems.length > 0 && | |||
There was a problem hiding this comment.
syntax: There's a typo in the variable name 'shoudShowSeparator' (missing 'l')
| visibleColumnsFieldMetadataItems.length > 0 && | |
| const shouldShowSeparator = | |
| visibleColumnsFieldMetadataItems.length > 0 && |
| currentViewWithCombinedFiltersAndSorts?.viewFilterGroups?.map( | ||
| (viewFilter) => viewFilter.id, | ||
| currentRecordFilterGroups?.map( | ||
| (recordFilterGroup) => recordFilterGroup.id, | ||
| ) ?? []; | ||
|
|
||
| for (const viewFilterGroupId of viewFilterGroupIds) { |
There was a problem hiding this comment.
style: Variable name viewFilterGroupId should be renamed to recordFilterGroupId for consistency with the new approach.
| const changeView = async (viewId: string) => { | ||
| setViewInUrl(viewId); | ||
| resetUnsavedViewStates(viewId); | ||
| }; |
There was a problem hiding this comment.
style: The function is still marked as async but no longer contains any awaited operations. Consider removing the async keyword.
| const changeView = async (viewId: string) => { | |
| setViewInUrl(viewId); | |
| resetUnsavedViewStates(viewId); | |
| }; | |
| const changeView = (viewId: string) => { | |
| setViewInUrl(viewId); | |
| }; |
| const viewFilterIdsToDelete = viewFilterGroupsToDelete.map( | ||
| (viewFilter) => viewFilter.id, | ||
| ); |
There was a problem hiding this comment.
syntax: Variable name viewFilter should be viewFilterGroup to match the actual type being mapped
| const viewFilterIdsToDelete = viewFilterGroupsToDelete.map( | |
| (viewFilter) => viewFilter.id, | |
| ); | |
| const viewFilterIdsToDelete = viewFilterGroupsToDelete.map( | |
| (viewFilterGroup) => viewFilterGroup.id, | |
| ); |
This PR implements CRUD for view filter groups with the new logic as already done for view filters and view sorts.
It also completely removes the old combined view filter group states and usage.
This PR is quite big but the impact is limited since it only changes advanced filters module, which is under feature flag at the moment, and it is already in a broken state so unusable, even if someone activates the feature flag.