Refactored record filter saving to view filters #9844

lucasbordeau · 2025-01-24T17:05:57Z

This PR refactors the record filter saving to view filters.

Before we used states to track the change of view filters, now we just check if there's a difference between the current record filters and the current view filters before saving.

We also use this check to show the reset and save buttons.

CRUD operations to perform on view filters are computed by utils , and .

Also added unit tests on those utils.

- Used this to create hooks to compare view filters and record filters - Added getCurrentViewOnly() hook - Created hook useSaveRecordFiltersToViewFilters - Created util to get CRUD operations to perform on view filters from record filters

greptile-apps

PR Summary

This PR refactors the view filter saving mechanism by replacing state-based tracking with direct comparisons between current record filters and view filters, introducing new utility functions and hooks for better modularity and maintainability.

Key changes:

Added useAreViewFiltersDifferentFromRecordFilters hook to efficiently compare current record filters with view filters
Introduced utility functions (getViewFiltersToCreate, getViewFiltersToDelete, getViewFiltersToUpdate) to handle CRUD operations based on filter differences
Implemented useSaveRecordFiltersToViewFilters hook to manage filter persistence using the new comparison-based approach
Added comprehensive unit tests for all new utility functions ensuring proper filter comparison logic
Simplified ViewBarDetails and UpdateViewButtonGroup components by removing state dependencies and using direct filter comparisons

_{17 file(s) reviewed, 15 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2025-01-24T17:06:59Z

packages/twenty-front/src/modules/views/components/ViewBarDetails.tsx

  const shouldExpandViewBar =
-    canPersistView ||
+    viewFiltersAreDifferentFromRecordFilters ||
    ((currentViewWithCombinedFiltersAndSorts?.viewSorts?.length ||
-      currentViewWithCombinedFiltersAndSorts?.viewFilters?.length) &&
+      currentRecordFilters?.length) &&
      isViewBarExpanded);


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

greptile-apps · 2025-01-24T17:07:01Z

packages/twenty-front/src/modules/views/hooks/useAreViewSortsDifferentFromRecordSorts.ts

+  const viewSortsAreDifferentFromRecordSorts = useRecoilComponentFamilyValueV2(
+    areViewSortsDifferentFromRecordSortsSelector,
+    { viewId: currentView?.id },
+  );


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

greptile-apps · 2025-01-24T17:07:10Z

packages/twenty-front/src/modules/views/components/UpdateViewButtonGroup.tsx

+  const canShowButton =
+    (viewFiltersAreDifferentFromRecordFilters ||
+      viewSortsAreDifferentFromRecordSorts) &&
+    !hasFiltersQueryParams;


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

greptile-apps · 2025-01-24T17:08:05Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewFiltersAndSorts.ts

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

+        await saveRecordFiltersToViewFilters();


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()

]);

greptile-apps · 2025-01-24T17:08:06Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewFiltersAndSorts.ts

+        await saveRecordFiltersToViewFilters();
+
        resetUnsavedViewStates(viewId);


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.

greptile-apps · 2025-01-24T17:09:04Z

packages/twenty-front/src/modules/views/utils/areViewFiltersEqual.ts

+  const propertiesToCompare: (keyof ViewFilter)[] = [
+    'displayValue',
+    'fieldMetadataId',
+    'viewFilterGroupId',
+    'operand',
+    'positionInViewFilterGroup',
+    'value',
+  ];


style: consider making this array constant and moving it outside the function to prevent recreation on each call

greptile-apps · 2025-01-24T17:09:05Z

packages/twenty-front/src/modules/views/utils/__tests__/getViewFiltersToDelete.test.ts

+  const baseFilter: ViewFilter = {
+    __typename: 'ViewFilter',
+    id: 'filter-1',
+    fieldMetadataId: 'field-1',
+    operand: ViewFilterOperand.Contains,
+    value: 'test',
+    displayValue: 'test',
+    viewFilterGroupId: 'group-1',
+    positionInViewFilterGroup: 0,
+  };


style: baseFilter test fixture could be moved to a shared test utils file since it's likely used across multiple test files

greptile-apps · 2025-01-24T17:09:53Z

packages/twenty-front/src/modules/views/utils/getViewFiltersToUpdate.ts

+    if (!isDefined(correspondingViewFilter)) {
+      return false;
+    }


logic: this early return should be removed - filters that don't exist in currentViewFilters should be created, not ignored

greptile-apps · 2025-01-24T17:09:53Z

packages/twenty-front/src/modules/views/utils/getViewFiltersToUpdate.ts

+  return newViewFilters.filter((newViewFilter) => {
+    const correspondingViewFilter = currentViewFilters.find(
+      (currentViewFilter) =>
+        currentViewFilter.fieldMetadataId === newViewFilter.fieldMetadataId &&
+        currentViewFilter.viewFilterGroupId === newViewFilter.viewFilterGroupId,
+    );


style: consider using a Map for O(1) lookup instead of find() for better performance with large filter sets

greptile-apps · 2025-01-24T17:10:05Z

packages/twenty-front/src/modules/views/utils/mapRecordFilterToViewFilter.ts

+  return {
+    __typename: 'ViewFilter',
+    ...recordFilter,
+  } satisfies ViewFilter;


style: Consider explicitly mapping each field instead of using spread operator to ensure type safety and catch breaking changes to either type.

charlesBochet · 2025-01-24T17:42:16Z

packages/twenty-front/src/modules/views/components/UpdateViewButtonGroup.tsx

@@ -89,7 +84,16 @@ export const UpdateViewButtonGroup = ({

  const { hasFiltersQueryParams } = useViewFromQueryParams();

-  const canShowButton = canPersistView && !hasFiltersQueryParams;
+  const { viewFiltersAreDifferentFromRecordFilters } =
+    useAreViewFiltersDifferentFromRecordFilters();


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!

charlesBochet · 2025-01-24T17:45:00Z

packages/twenty-front/src/modules/views/components/ViewBarDetails.tsx

-          (!viewFilter.variant || viewFilter.variant === 'default') &&
-          !viewFilter.viewFilterGroupId,
-      );
+  const otherViewFilters = useMemo(() => {


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

lucasbordeau added 2 commits January 24, 2025 18:04

Merge branch 'main' into refactor/record-filter-6

e2220cb

lucasbordeau requested a review from charlesBochet January 24, 2025 17:06

greptile-apps bot reviewed Jan 24, 2025

View reviewed changes

lucasbordeau added 2 commits January 24, 2025 18:26

Minor fixes

bf16a64

Merge branch 'main' into refactor/record-filter-6

860426c

charlesBochet reviewed Jan 24, 2025

View reviewed changes

charlesBochet approved these changes Jan 24, 2025

View reviewed changes

charlesBochet merged commit 570b2e3 into main Jan 24, 2025
47 checks passed

charlesBochet deleted the refactor/record-filter-6 branch January 24, 2025 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored record filter saving to view filters #9844

Refactored record filter saving to view filters #9844

lucasbordeau commented Jan 24, 2025

greptile-apps bot left a comment

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

greptile-apps bot Jan 24, 2025

charlesBochet Jan 24, 2025

charlesBochet Jan 24, 2025

charlesBochet Jan 24, 2025

		await saveRecordFiltersToViewFilters();

		resetUnsavedViewStates(viewId);

Refactored record filter saving to view filters #9844

Refactored record filter saving to view filters #9844

Conversation

lucasbordeau commented Jan 24, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

greptile-apps bot Jan 24, 2025

Choose a reason for hiding this comment

charlesBochet Jan 24, 2025

Choose a reason for hiding this comment

charlesBochet Jan 24, 2025

Choose a reason for hiding this comment

charlesBochet Jan 24, 2025

Choose a reason for hiding this comment