Clean record filter and record sort #10466

lucasbordeau · 2025-02-25T08:45:42Z

This PR removes what's left from record filter and record sort previous logic to handle CRUD and state management with view.

So everything that is named combinedFilter and combinedSort is removed here.

We implement currentRecordFilters and currentRecordSorts everywhere.

We also remove the event in a state onSortSelectComponentState. (a pattern we want to avoid)

- Removed unsaved states for view filters and view sorts - Use record filter and record sort related hooks and utils where needed instead

greptile-apps

PR Summary

This PR refactors the record filtering and sorting system by removing the legacy combined filter/sort implementation in favor of using currentRecordFilters and currentRecordSorts directly throughout the codebase.

Removes all useUpsertCombinedViewFilters/Sorts and useDeleteCombinedViewFilters/Sorts hooks, replacing them with direct useUpsertRecordFilter/Sort and useRemoveRecordFilter/Sort operations
Eliminates the event-in-state pattern by removing onSortSelectComponentState in favor of direct function calls
Adds new utility functions like getViewSortsToCreate, getViewSortsToUpdate, and getViewSortsToDelete with comprehensive test coverage
Implements useSaveRecordSortsToViewSorts to handle synchronization between record sorts and view sorts
Simplifies component implementations by accessing filter/sort state directly through currentRecordFiltersComponentState and currentRecordSortsComponentState

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

greptile-apps · 2025-02-25T08:47:13Z

.../modules/object-record/advanced-filter/components/AdvancedFilterRootLevelViewFilterGroup.tsx

@@ -49,7 +49,7 @@ export const AdvancedFilterRootLevelViewFilterGroup = ({
  return (
    <StyledContainer>
      {childViewFiltersAndViewFilterGroups.map((child, i) =>
-        child.__typename === 'ViewFilterGroup' ? (
+        (child as any).__typename === 'ViewFilterGroup' ? (


style: Using as any type assertion bypasses TypeScript's type checking. Consider creating a proper type guard function or interface to check if the object is a ViewFilterGroup instead.

greptile-apps · 2025-02-25T08:47:18Z

...t/src/modules/object-record/advanced-filter/components/AdvancedFilterRuleOptionsDropdown.tsx

      const childViewFilters = childViewFiltersAndViewFilterGroups.filter(
-        (child) => child.__typename === 'ViewFilter',
+        (child) => (child as any).__typename === 'ViewFilter',


style: Using (child as any).__typename is a type safety workaround. Consider defining a proper type for the child elements to avoid using any.

greptile-apps · 2025-02-25T08:47:19Z

...t/src/modules/object-record/advanced-filter/components/AdvancedFilterRuleOptionsDropdown.tsx

+
+  const currentRecordFilter = currentRecordFilters.find(
+    (recordFilter) => recordFilter.id === viewFilterId,
+  );

  const handleRemove = async () => {


style: Function is marked as async but doesn't use await anywhere.

Suggested change

const handleRemove = async () => {

const handleRemove = () => {

greptile-apps · 2025-02-25T08:48:43Z

...y-front/src/modules/object-record/object-filter-dropdown/components/AdvancedFilterButton.tsx

@@ -3,12 +3,12 @@ import { availableFieldMetadataItemsForFilterFamilySelector } from '@/object-met
 import { getFilterTypeFromFieldType } from '@/object-metadata/utils/formatFieldMetadataItemsAsFilterDefinitions';
 import { useUpsertCombinedViewFilterGroup } from '@/object-record/advanced-filter/hooks/useUpsertCombinedViewFilterGroup';


style: This hook useUpsertCombinedViewFilterGroup still has 'Combined' in its name while other combined hooks are being removed. Consider renaming for consistency with the PR's goal of removing combined filter/sort implementations.

greptile-apps · 2025-02-25T08:49:21Z

...modules/object-record/object-filter-dropdown/components/ObjectFilterDropdownSourceSelect.tsx

  // TODO: this should be removed as it is not consistent across re-renders
  const [fieldId] = useState(v4());


style: This TODO comment about fieldId being inconsistent across re-renders remains unaddressed in this PR. Consider replacing this with a more stable ID generation approach.

greptile-apps · 2025-02-25T08:50:49Z

...-front/src/modules/object-record/record-index/components/RecordIndexTableContainerEffect.tsx

  useEffect(() => {
-    currentViewWithSavedFiltersAndSorts?.viewFields.forEach((viewField) => {
+    currentView?.viewFields.forEach((viewField) => {
      setViewFieldAggregateOperation(viewField);
    });
-  }, [
-    currentViewWithSavedFiltersAndSorts?.viewFields,
-    setViewFieldAggregateOperation,
-  ]);
+  }, [currentView, setViewFieldAggregateOperation]);


style: This effect now depends on currentView instead of currentViewWithSavedFiltersAndSorts?.viewFields. Make sure this doesn't cause unnecessary re-renders if other properties of currentView change but viewFields doesn't.

greptile-apps · 2025-02-25T08:53:06Z

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

  }, [
    applyViewFiltersToCurrentRecordFilters,
    getFiltersFromQueryParams,
    hasFiltersQueryParams,
    resetUnsavedViewStates,
-    setUnsavedViewFilter,
  ]);


logic: resetUnsavedViewStates is included in the dependency array but not used in the effect

Suggested change

}, [

applyViewFiltersToCurrentRecordFilters,

getFiltersFromQueryParams,

hasFiltersQueryParams,

resetUnsavedViewStates,

setUnsavedViewFilter,

]);

}, [

applyViewFiltersToCurrentRecordFilters,

getFiltersFromQueryParams,

hasFiltersQueryParams,

]);

greptile-apps · 2025-02-25T08:54:24Z

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

+    if (!advancedRecordFilterIds) {
      throw new Error('No advanced view filters to remove');


style: The error message still refers to 'advanced view filters' but the code is now checking for 'advancedRecordFilterIds'. Consider updating the error message to 'No advanced record filters to remove' for consistency.

greptile-apps · 2025-02-25T08:54:38Z

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

      const { selectedRecordIds } = jsonRelationFilterValueSchema
        .catch({
          isCurrentWorkspaceMemberSelected: false,
          selectedRecordIds: simpleRelationFilterValueSchema.parse(
-            viewFilterUsedInDropdown?.value,
+            recordFilterUsedInDropdown?.value,
          ),
        })
-        .parse(viewFilterUsedInDropdown?.value);
+        .parse(recordFilterUsedInDropdown?.value);



logic: This code assumes recordFilterUsedInDropdown could be undefined when parsing its value. If currentRecordFilters is empty, this will cause a runtime error when trying to parse undefined.

Suggested change

const { selectedRecordIds } = jsonRelationFilterValueSchema

.catch({

isCurrentWorkspaceMemberSelected: false,

selectedRecordIds: simpleRelationFilterValueSchema.parse(

viewFilterUsedInDropdown?.value,

recordFilterUsedInDropdown?.value,

),

})

.parse(viewFilterUsedInDropdown?.value);

.parse(recordFilterUsedInDropdown?.value);

const { selectedRecordIds } = jsonRelationFilterValueSchema

.catch({

isCurrentWorkspaceMemberSelected: false,

selectedRecordIds: simpleRelationFilterValueSchema.parse(

recordFilterUsedInDropdown?.value,

),

})

.parse(recordFilterUsedInDropdown?.value ?? null);

greptile-apps · 2025-02-25T08:54:39Z

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

+      const recordFilterSelectedRecords = isNonEmptyString(
+        recordFilterUsedInDropdown?.value,
      )
-        ? JSON.parse(viewFilterUsedInDropdown.value)
+        ? JSON.parse(recordFilterUsedInDropdown.value)
        : [];


logic: Similar issue here - if recordFilterUsedInDropdown is undefined, attempting to access recordFilterUsedInDropdown.value on line 71 will cause a runtime error.

Suggested change

const recordFilterSelectedRecords = isNonEmptyString(

recordFilterUsedInDropdown?.value,

)

? JSON.parse(viewFilterUsedInDropdown.value)

? JSON.parse(recordFilterUsedInDropdown.value)

: [];

const recordFilterSelectedRecords = isNonEmptyString(

recordFilterUsedInDropdown?.value,

)

? JSON.parse(recordFilterUsedInDropdown?.value ?? '')

: [];

charlesBochet · 2025-02-25T09:06:31Z

.../modules/object-record/advanced-filter/components/AdvancedFilterRootLevelViewFilterGroup.tsx

@@ -49,7 +49,7 @@ export const AdvancedFilterRootLevelViewFilterGroup = ({
  return (
    <StyledContainer>
      {childViewFiltersAndViewFilterGroups.map((child, i) =>
-        child.__typename === 'ViewFilterGroup' ? (
+        (child as any).__typename === 'ViewFilterGroup' ? (


can we avoid it?

I'll try yes

- Fixed bug with aggregate not taking current record filters

lucasbordeau added 2 commits February 24, 2025 21:40

- Removed combinedFilter and combinedSort related hooks and utils

92b8bfc

- Removed unsaved states for view filters and view sorts - Use record filter and record sort related hooks and utils where needed instead

Merge branch 'main' into refactor/record-filter-and-sort-cleanup

a20dce5

greptile-apps bot reviewed Feb 25, 2025

View reviewed changes

charlesBochet reviewed Feb 25, 2025

View reviewed changes

charlesBochet approved these changes Feb 25, 2025

View reviewed changes

lucasbordeau added 2 commits February 25, 2025 14:52

- Fixed bug with sort not taken into account

d35d1d1

- Fixed bug with aggregate not taking current record filters

Merge branch 'main' into refactor/record-filter-and-sort-cleanup

88870d7

lucasbordeau enabled auto-merge (squash) February 25, 2025 13:53

lucasbordeau merged commit 2247d3f into main Feb 25, 2025
45 checks passed

lucasbordeau deleted the refactor/record-filter-and-sort-cleanup branch February 25, 2025 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean record filter and record sort #10466

Clean record filter and record sort #10466

lucasbordeau commented Feb 25, 2025

greptile-apps bot left a comment

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

greptile-apps bot Feb 25, 2025

charlesBochet Feb 25, 2025

lucasbordeau Feb 25, 2025

	const handleRemove = async () => {
	const handleRemove = () => {

		@@ -3,12 +3,12 @@ import { availableFieldMetadataItemsForFilterFamilySelector } from '@/object-met
		import { getFilterTypeFromFieldType } from '@/object-metadata/utils/formatFieldMetadataItemsAsFilterDefinitions';
		import { useUpsertCombinedViewFilterGroup } from '@/object-record/advanced-filter/hooks/useUpsertCombinedViewFilterGroup';

		// TODO: this should be removed as it is not consistent across re-renders
		const [fieldId] = useState(v4());

		if (!advancedRecordFilterIds) {
		throw new Error('No advanced view filters to remove');

Clean record filter and record sort #10466

Clean record filter and record sort #10466

Conversation

lucasbordeau commented Feb 25, 2025

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

greptile-apps bot Feb 25, 2025

Choose a reason for hiding this comment

charlesBochet Feb 25, 2025

Choose a reason for hiding this comment

lucasbordeau Feb 25, 2025

Choose a reason for hiding this comment