Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored record filter saving to view filters #9844

Merged
merged 4 commits into from
Jan 24, 2025
Merged

Refactored record filter saving to view filters #9844

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e71e86
- Added util and its test to compare view filters
lucasbordeau Jan 24, 2025
e2220cb
Merge branch 'main' into refactor/record-filter-6
lucasbordeau Jan 24, 2025
bf16a64
Minor fixes
lucasbordeau Jan 24, 2025
860426c
Merge branch 'main' into refactor/record-filter-6
lucasbordeau Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions packages/twenty-front/src/modules/views/components/UpdateViewButtonGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ import { Dropdown } from '@/ui/layout/dropdown/components/Dropdown';
import { DropdownMenuItemsContainer } from '@/ui/layout/dropdown/components/DropdownMenuItemsContainer';
import { useDropdown } from '@/ui/layout/dropdown/hooks/useDropdown';
import { HotkeyScope } from '@/ui/utilities/hotkey/types/HotkeyScope';
import { useRecoilComponentFamilyValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentFamilyValueV2';
import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2';
import { useSetRecoilComponentStateV2 } from '@/ui/utilities/state/component-state/hooks/useSetRecoilComponentStateV2';
import { UPDATE_VIEW_BUTTON_DROPDOWN_ID } from '@/views/constants/UpdateViewButtonDropdownId';
import { useViewFromQueryParams } from '@/views/hooks/internal/useViewFromQueryParams';
import { useAreViewFiltersDifferentFromRecordFilters } from '@/views/hooks/useAreViewFiltersDifferentFromRecordFilters';
import { useAreViewSortsDifferentFromRecordSorts } from '@/views/hooks/useAreViewSortsDifferentFromRecordSorts';
import { useGetCurrentView } from '@/views/hooks/useGetCurrentView';
import { useSaveCurrentViewFiltersAndSorts } from '@/views/hooks/useSaveCurrentViewFiltersAndSorts';
import { currentViewIdComponentState } from '@/views/states/currentViewIdComponentState';
import { canPersistViewComponentFamilySelector } from '@/views/states/selectors/canPersistViewComponentFamilySelector';
import { VIEW_PICKER_DROPDOWN_ID } from '@/views/view-picker/constants/ViewPickerDropdownId';
import { useViewPickerMode } from '@/views/view-picker/hooks/useViewPickerMode';
import { viewPickerReferenceViewIdComponentState } from '@/views/view-picker/states/viewPickerReferenceViewIdComponentState';
Expand All @@ -46,11 +46,6 @@ export const UpdateViewButtonGroup = ({

const currentViewId = useRecoilComponentValueV2(currentViewIdComponentState);

const canPersistView = useRecoilComponentFamilyValueV2(
canPersistViewComponentFamilySelector,
{ viewId: currentViewId },
);

const { closeDropdown: closeUpdateViewButtonDropdown } = useDropdown(
UPDATE_VIEW_BUTTON_DROPDOWN_ID,
);
Expand Down Expand Up @@ -89,7 +84,16 @@ export const UpdateViewButtonGroup = ({

const { hasFiltersQueryParams } = useViewFromQueryParams();

const canShowButton = canPersistView && !hasFiltersQueryParams;
const { viewFiltersAreDifferentFromRecordFilters } =
useAreViewFiltersDifferentFromRecordFilters();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idea: we could also justify returning a diff (could be more versatile in the usage) and the condition would be diff.length > 0

but happy with the current hook!

Copy link
Contributor Author

@lucasbordeau lucasbordeau Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always better to have one hook that does one precise job than creating a low-level Swiss army hook.


const { viewSortsAreDifferentFromRecordSorts } =
useAreViewSortsDifferentFromRecordSorts();

const canShowButton =
(viewFiltersAreDifferentFromRecordFilters ||
viewSortsAreDifferentFromRecordSorts) &&
!hasFiltersQueryParams;
Comment on lines +93 to +96
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Consider adding a check for currentViewId here - the button could show up even when there's no current view selected


if (!canShowButton) {
return <></>;
Expand Down
83 changes: 34 additions & 49 deletions packages/twenty-front/src/modules/views/components/ViewBarDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,23 @@ import { ReactNode, useMemo } from 'react';
import { useObjectNameSingularFromPlural } from '@/object-metadata/hooks/useObjectNameSingularFromPlural';
import { AddObjectFilterFromDetailsButton } from '@/object-record/object-filter-dropdown/components/AddObjectFilterFromDetailsButton';
import { ObjectFilterDropdownComponentInstanceContext } from '@/object-record/object-filter-dropdown/states/contexts/ObjectFilterDropdownComponentInstanceContext';
import { RecordFilter } from '@/object-record/record-filter/types/RecordFilter';
import { useHandleToggleTrashColumnFilter } from '@/object-record/record-index/hooks/useHandleToggleTrashColumnFilter';
import { DropdownScope } from '@/ui/layout/dropdown/scopes/DropdownScope';
import { useRecoilComponentFamilyValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentFamilyValueV2';
import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2';
import { AdvancedFilterDropdownButton } from '@/views/components/AdvancedFilterDropdownButton';
import { EditableFilterDropdownButton } from '@/views/components/EditableFilterDropdownButton';
import { EditableSortChip } from '@/views/components/EditableSortChip';
import { ViewBarFilterEffect } from '@/views/components/ViewBarFilterEffect';
import { useViewFromQueryParams } from '@/views/hooks/internal/useViewFromQueryParams';

import { currentRecordFiltersComponentState } from '@/object-record/record-filter/states/currentRecordFiltersComponentState';
import { useApplyCurrentViewFiltersToCurrentRecordFilters } from '@/views/hooks/useApplyCurrentViewFiltersToCurrentRecordFilters';
import { useAreViewFiltersDifferentFromRecordFilters } from '@/views/hooks/useAreViewFiltersDifferentFromRecordFilters';
import { useAreViewSortsDifferentFromRecordSorts } from '@/views/hooks/useAreViewSortsDifferentFromRecordSorts';
import { useGetCurrentView } from '@/views/hooks/useGetCurrentView';
import { useResetUnsavedViewStates } from '@/views/hooks/useResetUnsavedViewStates';
import { availableFilterDefinitionsComponentState } from '@/views/states/availableFilterDefinitionsComponentState';
import { availableSortDefinitionsComponentState } from '@/views/states/availableSortDefinitionsComponentState';
import { isViewBarExpandedComponentState } from '@/views/states/isViewBarExpandedComponentState';
import { canPersistViewComponentFamilySelector } from '@/views/states/selectors/canPersistViewComponentFamilySelector';
import { mapViewFiltersToFilters } from '@/views/utils/mapViewFiltersToFilters';
import { mapViewSortsToSorts } from '@/views/utils/mapViewSortsToSorts';
import { isDefined } from 'twenty-ui';
import { VariantFilterChip } from './VariantFilterChip';
Expand Down Expand Up @@ -118,13 +116,8 @@ export const ViewBarDetails = ({

const { hasFiltersQueryParams } = useViewFromQueryParams();

const canPersistView = useRecoilComponentFamilyValueV2(
canPersistViewComponentFamilySelector,
{ viewId },
);

const availableFilterDefinitions = useRecoilComponentValueV2(
availableFilterDefinitionsComponentState,
const currentRecordFilters = useRecoilComponentValueV2(
currentRecordFiltersComponentState,
);

const availableSortDefinitions = useRecoilComponentValueV2(
Expand All @@ -139,35 +132,34 @@ export const ViewBarDetails = ({
viewBarId: viewBarId,
});
const { resetUnsavedViewStates } = useResetUnsavedViewStates();
const canResetView = canPersistView && !hasFiltersQueryParams;

const { otherViewFilters, defaultViewFilters } = useMemo(() => {
if (!currentViewWithCombinedFiltersAndSorts) {
return {
otherViewFilters: [],
defaultViewFilters: [],
};
}
const { viewFiltersAreDifferentFromRecordFilters } =
useAreViewFiltersDifferentFromRecordFilters();

const { viewSortsAreDifferentFromRecordSorts } =
useAreViewSortsDifferentFromRecordSorts();

const canResetView =
(viewFiltersAreDifferentFromRecordFilters ||
viewSortsAreDifferentFromRecordSorts) &&
!hasFiltersQueryParams;

const otherViewFilters =
currentViewWithCombinedFiltersAndSorts.viewFilters.filter(
(viewFilter) =>
viewFilter.variant &&
viewFilter.variant !== 'default' &&
!viewFilter.viewFilterGroupId,
);
const defaultViewFilters =
currentViewWithCombinedFiltersAndSorts.viewFilters.filter(
(viewFilter) =>
(!viewFilter.variant || viewFilter.variant === 'default') &&
!viewFilter.viewFilterGroupId,
);
const otherViewFilters = useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neat: not a big fan of useMemo => I would rather investigate the re-renders root cause

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just split the actual hook, I think it's not necessary too, I can remove it indeed, useMemo is only useful for performance optimizations not re-renders.

return currentRecordFilters.filter(
(viewFilter) =>
viewFilter.variant &&
viewFilter.variant !== 'default' &&
!viewFilter.viewFilterGroupId,
);
}, [currentRecordFilters]);

return {
otherViewFilters,
defaultViewFilters,
};
}, [currentViewWithCombinedFiltersAndSorts]);
const defaultViewFilters = useMemo(() => {
return currentRecordFilters.filter(
(viewFilter) =>
(!viewFilter.variant || viewFilter.variant === 'default') &&
!viewFilter.viewFilterGroupId,
);
}, [currentRecordFilters]);

const { applyCurrentViewFiltersToCurrentRecordFilters } =
useApplyCurrentViewFiltersToCurrentRecordFilters();
Expand All @@ -181,9 +173,9 @@ export const ViewBarDetails = ({
};

const shouldExpandViewBar =
canPersistView ||
viewFiltersAreDifferentFromRecordFilters ||
((currentViewWithCombinedFiltersAndSorts?.viewSorts?.length ||
currentViewWithCombinedFiltersAndSorts?.viewFilters?.length) &&
currentRecordFilters?.length) &&
isViewBarExpanded);
Comment on lines 175 to 179
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: shouldExpandViewBar logic could be simplified by combining conditions and using optional chaining


if (!shouldExpandViewBar) {
Expand All @@ -201,11 +193,7 @@ export const ViewBarDetails = ({
{otherViewFilters.map((viewFilter) => (
<VariantFilterChip
key={viewFilter.fieldMetadataId}
// Why do we have two types, Filter and ViewFilter?
// Why key defition is already present in the Filter type and added on the fly here with mapViewFiltersToFilters ?
// Also as filter is spread into viewFilter, definition is present
// FixMe: Ugly hack to make it work
viewFilter={viewFilter as unknown as RecordFilter}
viewFilter={viewFilter}
viewBarId={viewBarId}
/>
))}
Expand All @@ -228,10 +216,7 @@ export const ViewBarDetails = ({
</StyledSeperatorContainer>
)}
{showAdvancedFilterDropdownButton && <AdvancedFilterDropdownButton />}
{mapViewFiltersToFilters(
defaultViewFilters,
availableFilterDefinitions,
).map((viewFilter) => (
{defaultViewFilters.map((viewFilter) => (
<ObjectFilterDropdownComponentInstanceContext.Provider
key={viewFilter.id}
value={{ instanceId: viewFilter.id }}
Expand Down
47 changes: 47 additions & 0 deletions packages/twenty-front/src/modules/views/hooks/useAreViewFiltersDifferentFromRecordFilters.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { currentRecordFiltersComponentState } from '@/object-record/record-filter/states/currentRecordFiltersComponentState';
import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2';
import { useGetCurrentViewOnly } from '@/views/hooks/useGetCurrentViewOnly';
import { getViewFiltersToCreate } from '@/views/utils/getViewFiltersToCreate';
import { getViewFiltersToDelete } from '@/views/utils/getViewFiltersToDelete';
import { getViewFiltersToUpdate } from '@/views/utils/getViewFiltersToUpdate';
import { mapRecordFilterToViewFilter } from '@/views/utils/mapRecordFilterToViewFilter';
import { useMemo } from 'react';

export const useAreViewFiltersDifferentFromRecordFilters = () => {
const { currentView } = useGetCurrentViewOnly();
const currentRecordFilters = useRecoilComponentValueV2(
currentRecordFiltersComponentState,
);

const viewFiltersAreDifferentFromRecordFilters = useMemo(() => {
const currentViewFilters = currentView?.viewFilters ?? [];
const viewFiltersFromCurrentRecordFilters = currentRecordFilters.map(
mapRecordFilterToViewFilter,
);

const viewFiltersToCreate = getViewFiltersToCreate(
currentViewFilters,
viewFiltersFromCurrentRecordFilters,
);

const viewFiltersToDelete = getViewFiltersToDelete(
currentViewFilters,
viewFiltersFromCurrentRecordFilters,
);

const viewFiltersToUpdate = getViewFiltersToUpdate(
currentViewFilters,
viewFiltersFromCurrentRecordFilters,
);

const filtersHaveChanged =
viewFiltersToCreate.length > 0 ||
viewFiltersToDelete.length > 0 ||
viewFiltersToUpdate.length > 0;

// TODO: this is temporary, record sorts need to be refactored
return filtersHaveChanged;
}, [currentRecordFilters, currentView]);

return { viewFiltersAreDifferentFromRecordFilters };
};
14 changes: 14 additions & 0 deletions packages/twenty-front/src/modules/views/hooks/useAreViewSortsDifferentFromRecordSorts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { useRecoilComponentFamilyValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentFamilyValueV2';
import { useGetCurrentViewOnly } from '@/views/hooks/useGetCurrentViewOnly';
import { areViewSortsDifferentFromRecordSortsSelector } from '@/views/states/selectors/areViewSortsDifferentFromRecordSortsFamilySelector';

export const useAreViewSortsDifferentFromRecordSorts = () => {
const { currentView } = useGetCurrentViewOnly();

const viewSortsAreDifferentFromRecordSorts = useRecoilComponentFamilyValueV2(
areViewSortsDifferentFromRecordSortsSelector,
{ viewId: currentView?.id },
);
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: hook will trigger recomputation on every currentView change, even if id hasn't changed. Consider memoizing the config object


return { viewSortsAreDifferentFromRecordSorts };
};
22 changes: 22 additions & 0 deletions packages/twenty-front/src/modules/views/hooks/useGetCurrentViewOnly.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { usePrefetchedData } from '@/prefetch/hooks/usePrefetchedData';
import { PrefetchKey } from '@/prefetch/types/PrefetchKey';
import { useRecoilComponentValueV2 } from '@/ui/utilities/state/component-state/hooks/useRecoilComponentValueV2';
import { currentViewIdComponentState } from '@/views/states/currentViewIdComponentState';
import { View } from '@/views/types/View';

import { useMemo } from 'react';

export const useGetCurrentViewOnly = () => {
const { records: views } = usePrefetchedData<View>(PrefetchKey.AllViews);

const currentViewId = useRecoilComponentValueV2(currentViewIdComponentState);

const currentView = useMemo(
() => views.find((view) => view.id === currentViewId),
[views, currentViewId],
);

return {
currentView,
};
};
58 changes: 9 additions & 49 deletions packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewFiltersAndSorts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { usePersistViewFilterRecords } from '@/views/hooks/internal/usePersistVi
import { usePersistViewSortRecords } from '@/views/hooks/internal/usePersistViewSortRecords';
import { useGetViewFromCache } from '@/views/hooks/useGetViewFromCache';
import { useResetUnsavedViewStates } from '@/views/hooks/useResetUnsavedViewStates';
import { useSaveRecordFiltersToViewFilters } from '@/views/hooks/useSaveRecordFiltersToViewFilters';
import { currentViewIdComponentState } from '@/views/states/currentViewIdComponentState';
import { unsavedToDeleteViewFilterGroupIdsComponentFamilyState } from '@/views/states/unsavedToDeleteViewFilterGroupIdsComponentFamilyState';
import { unsavedToDeleteViewFilterIdsComponentFamilyState } from '@/views/states/unsavedToDeleteViewFilterIdsComponentFamilyState';
Expand Down Expand Up @@ -130,53 +131,6 @@ export const useSaveCurrentViewFiltersAndSorts = (
],
);

const saveViewFilters = useRecoilCallback(
({ snapshot }) =>
async (viewId: string) => {
const unsavedToDeleteViewFilterIds = getSnapshotValue(
snapshot,
unsavedToDeleteViewFilterIdsCallbackState({ viewId }),
);

const unsavedToUpsertViewFilters = getSnapshotValue(
snapshot,
unsavedToUpsertViewFiltersCallbackState({ viewId }),
);

const view = await getViewFromCache(viewId);

if (isUndefinedOrNull(view)) {
return;
}

const viewFiltersToCreate = unsavedToUpsertViewFilters.filter(
(viewFilter) =>
!view.viewFilters.some(
(viewFilterToFilter) => viewFilterToFilter.id === viewFilter.id,
),
);

const viewFiltersToUpdate = unsavedToUpsertViewFilters.filter(
(viewFilter) =>
view.viewFilters.some(
(viewFilterToFilter) => viewFilterToFilter.id === viewFilter.id,
),
);

await createViewFilterRecords(viewFiltersToCreate, view);
await updateViewFilterRecords(viewFiltersToUpdate);
await deleteViewFilterRecords(unsavedToDeleteViewFilterIds);
},
[
createViewFilterRecords,
deleteViewFilterRecords,
getViewFromCache,
unsavedToDeleteViewFilterIdsCallbackState,
unsavedToUpsertViewFiltersCallbackState,
updateViewFilterRecords,
],
);

const saveViewFilterGroups = useRecoilCallback(
({ snapshot }) =>
async (viewId: string) => {
Expand Down Expand Up @@ -226,6 +180,9 @@ export const useSaveCurrentViewFiltersAndSorts = (
],
);

const { saveRecordFiltersToViewFilters } =
useSaveRecordFiltersToViewFilters();

const saveCurrentViewFilterAndSorts = useRecoilCallback(
({ snapshot }) =>
async (viewIdFromProps?: string) => {
Expand All @@ -240,17 +197,20 @@ export const useSaveCurrentViewFiltersAndSorts = (
const viewId = viewIdFromProps ?? currentViewId;

await saveViewFilterGroups(viewId);
await saveViewFilters(viewId);
// await saveViewFilters(viewId);
await saveViewSorts(viewId);

await saveRecordFiltersToViewFilters();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: These operations are being executed sequentially but are independent. Consider using Promise.all() to run them concurrently for better performance:

Suggested change
await saveViewFilterGroups(viewId);
await saveViewFilters(viewId);
// await saveViewFilters(viewId);
await saveViewSorts(viewId);
await saveRecordFiltersToViewFilters();
await Promise.all([
saveViewFilterGroups(viewId),
saveViewSorts(viewId),
saveRecordFiltersToViewFilters()
]);


resetUnsavedViewStates(viewId);
Comment on lines +181 to 183
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: resetUnsavedViewStates is called after all save operations complete, but there's no error handling. If any save operation fails, the states may be incorrectly reset.

},
[
currentViewIdCallbackState,
resetUnsavedViewStates,
saveViewFilters,
// saveViewFilters,
saveViewSorts,
saveViewFilterGroups,
saveRecordFiltersToViewFilters,
],
);

Expand Down
Loading
Loading