Remove filter definition #10227
Remove filter definition #10227
Conversation
- Removed filter definition states
![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 significantly refactors the filtering system by removing filter definitions and deriving filter behavior directly from field metadata items. Here's a concise summary of the key changes:
- Removes all filter definition states and utilities, transitioning to direct usage of fieldMetadataItem properties
- Replaces
formatFieldMetadataItemAsFilterDefinitionwith direct usage ofgetFilterTypeFromFieldType - Modifies
RecordFiltertype to include required 'type' and 'label' fields instead of optional 'definition' - Updates filter-related components to derive filter information directly from field metadata
- Simplifies data flow by removing intermediate filter definition transformation layer
Key points to review:
- Check error handling for missing field metadata items, particularly in
mapViewFiltersToFilters.ts - Verify filter behavior consistency across all field types (TEXT, NUMBER, DATE, etc.)
- Review the fieldId state variable marked as TODO in ObjectFilterDropdownSourceSelect
- Consider impact on type safety with removal of filter definition layer
- Test backward compatibility with existing filters and views
50 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings | Greptile
| const filterType = getFilterTypeFromFieldType( | ||
| defaultFieldMetadataItem.type, | ||
| ); |
There was a problem hiding this comment.
style: Consider adding error handling for unsupported field types in getFilterTypeFromFieldType
| type: getFilterTypeFromFieldType(fieldMetadataItemUsedInDropdown.type), | ||
| label: fieldMetadataItemUsedInDropdown.label, |
There was a problem hiding this comment.
style: Duplicate code from lines 63-64. Consider extracting type and label into variables to avoid repetition
| const filterDefinition = formatFieldMetadataItemAsFilterDefinition({ | ||
| field: fieldMetadataItemUsedInDropdown, | ||
| }); | ||
|
|
||
| applyRecordFilter({ | ||
| id: selectedFilter?.id ? selectedFilter.id : v4(), | ||
| fieldMetadataId: fieldMetadataItemUsedInDropdown?.id ?? '', |
There was a problem hiding this comment.
style: Optional chaining with nullish coalescing here could be simplified to just fieldMetadataItemUsedInDropdown.id since line 45 ensures it exists
| fieldMetadataId: fieldMetadataItemUsedInDropdown?.id ?? '', | |
| fieldMetadataId: fieldMetadataItemUsedInDropdown.id, |
| @@ -28,13 +21,8 @@ export const SingleEntityObjectFilterDropdownButtonEffect = () => { | |||
|
|
|||
| const firstFieldMetadataItem = filterableFieldMetadataItems[0]; | |||
There was a problem hiding this comment.
logic: No null check on filterableFieldMetadataItems[0]. Could cause runtime errors if array is empty.
| const firstFieldMetadataItem = filterableFieldMetadataItems[0]; | |
| const firstFieldMetadataItem = filterableFieldMetadataItems[0]; | |
| if (!firstFieldMetadataItem) return null; |
| @@ -56,19 +56,19 @@ export const useResetFilterDropdown = (componentInstanceId?: string) => { | |||
| set(objectFilterDropdownSearchInputCallbackState, ''); | |||
| set(objectFilterDropdownSelectedRecordIdsCallbackState, []); | |||
| set(selectedFilterCallbackState, undefined); | |||
There was a problem hiding this comment.
style: selectedFilterCallbackState is set to undefined while other states use null. Consider standardizing to either null or undefined across all states.
| displayAsRelativeDate: true, | ||
| }, | ||
| }, | ||
| position: expect.any(Number), | ||
| position: 3, |
There was a problem hiding this comment.
style: Hardcoding position to 3 could make tests brittle. Consider using expect.any(Number) or documenting why 3 is the expected value.
| const setFieldMetadataItemIdUsedInDropdown = useSetRecoilComponentStateV2( | ||
| fieldMetadataItemIdUsedInDropdownComponentState, | ||
| ); |
There was a problem hiding this comment.
logic: setFieldMetadataItemIdUsedInDropdown is missing viewFilterDropdownId parameter, unlike other state setters. This inconsistency could cause state management issues.
| label: mockFieldMetadataItem.label, | ||
| type: getFilterTypeFromFieldType(mockFieldMetadataItem.type), |
There was a problem hiding this comment.
logic: label is now derived from fieldMetadataItem.label instead of mockViewFilter.displayValue, which could potentially cause issues if these values differ in production
| if (isDefined(currentView)) { | ||
| setCurrentRecordFilters( | ||
| mapViewFiltersToFilters(currentView.viewFilters, filterDefinitions), | ||
| mapViewFiltersToFilters( | ||
| currentView.viewFilters, | ||
| filterableFieldMetadataItems, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: ensure that all field metadata items needed for filtering are available before setting filters, as missing metadata could cause runtime errors
| if (!isDefined(availableFieldMetadataItem)) { | ||
| throw new Error( | ||
| `Field metadata item not found for view filter ${viewFilter.id} and field metadata id ${viewFilter.fieldMetadataId}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: Throwing an error here could crash the application if metadata is missing. Consider returning null instead to maintain previous behavior.
- Fixed jest config to remove the many useless console.error
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This PR removes completely filter definition usage and states, everything is now derived from fieldMetadataItem.