Implemented view filter group CRUD hooks and utils #10551
Conversation
…ups and its unit test - ViewBar CRUD buttons are now taking view filter groups into account - Implemented getViewFilterGroupsToCreate and its unit test - Implemented getViewFilterGroupsToDelete and its unit test - Implemented getViewFilterGroupsToUpdate and its unit test - Implemented compareStrictlyExceptForNullAndUndefined and its unit test - Refactored nullish comparisons with compareStrictlyExceptForNullAndUndefined
| .getValue(); | ||
|
|
||
| if (isDefined(currentView)) { | ||
| setCurrentRecordFilterGroups( |
There was a problem hiding this comment.
to avoid re-render, I would encourage to check (by deepEqual) if new != old
There was a problem hiding this comment.
I'll implement it
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implemented view filter group CRUD hooks and utilities for managing filter groups in the Twenty application.
- Added
useAreViewFilterGroupsDifferentFromRecordFilterGroupshook in/packages/twenty-front/src/modules/views/hooks/useAreViewFilterGroupsDifferentFromRecordFilterGroups.tsto detect differences between view filter groups and record filter groups - Created utility functions
getViewFilterGroupsToCreate,getViewFilterGroupsToDelete, andgetViewFilterGroupsToUpdateto handle CRUD operations for filter groups - Implemented
compareStrictlyExceptForNullAndUndefinedutility in/packages/twenty-front/src/utils/compareStrictlyExceptForNullAndUndefined.tsto safely compare values that might be null or undefined - Added
useApplyCurrentViewFilterGroupsToCurrentRecordFilterGroupshook to synchronize view filter groups with record filter groups - Integrated filter group comparison in
ViewBarDetailsandUpdateViewButtonGroupcomponents to properly handle unsaved changes
13 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| if (!isDefined(currentView)) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
style: Consider returning false instead of undefined when currentView is not defined. This would make the return type consistent (boolean) and avoid potential issues with boolean checks elsewhere in the codebase.
| const shouldUpdateBecauseViewFilterIsDifferent = !areViewFilterGroupsEqual( | ||
| newViewFilterGroup, | ||
| correspondingViewFilterGroup, | ||
| ); |
There was a problem hiding this comment.
style: The variable name suggests this is checking if a filter is different, but it's actually checking if a filter group is different.
| currentRecordFilterGroupsComponentState, | ||
| ); | ||
|
|
||
| const viewFilterGroupsAreDifferentFromRecordFilterGroups = useMemo(() => { |
There was a problem hiding this comment.
is this useMemo useful?
This hook is taking dependecy on currentView (prefetchViewFromViewIdFamilySelector behind the scene) + currentRecordFilterGroups which seems to be perfect
There was a problem hiding this comment.
I can remove it yes
This PR implements hooks and utils logic for handling CRUD and view filter group comparison.
The main hook is useAreViewFilterGroupsDifferentFromRecordFilterGroups, like view filters and view sorts.
Inside this hook we implement getViewFilterGroupsToCreate, getViewFilterGroupsToDelete and getViewFilterGroupsToUpdate.
All of those come with their unit tests.
In this PR we also introduce a new util to prevent nasty bugs happening when we compare undefined === null,
This util is called compareStrictlyExceptForNullAndUndefined and it should replace every strict equality comparison between values that can be null or undefined (which we have a lot)
This could be enforced by a custom ESLint rule, the autofix may also be implemented (maybe the util should be put in twenty-shared ?)