Use search in multi object pickers #7909
Conversation
![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 pull request enhances the search functionality in multi-object pickers by introducing a new combined search records query and refactoring existing hooks.
- Added
useGenerateCombinedSearchRecordsQueryhook inpackages/twenty-front/src/modules/object-record/multiple-objects/hooks/useGenerateCombinedSearchRecordsQuery.tsfor generating dynamic GraphQL queries across multiple object types - Introduced
isObjectMetadataItemSearchableutility inpackages/twenty-front/src/modules/object-record/multiple-objects/hooks/utils/isObjectMetadataItemSearchable.tsto determine searchable objects - Modified
useMultiObjectSearchMatchesSearchFilterAndSelectedItemsQueryanduseMultiObjectSearchMatchesSearchFilterAndToSelectQueryhooks to integrate the new search parameter and query generation - Removed
useSearchFilterPerMetadataItemhook, indicating a significant refactor in search logic - Implemented
formatSearchResultsfunction to process and standardize search results across different object types
5 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
| query CombinedSearchRecords( | ||
| ${filterPerMetadataItemArray}, | ||
| ${limitPerMetadataItemArray}, | ||
| $search: String, |
There was a problem hiding this comment.
style: Consider adding a non-null constraint to the $search parameter if it's required
| export const isObjectMetadataItemSearchable = ( | ||
| objectMetadataItem: ObjectMetadataItem, | ||
| ) => { | ||
| return ( | ||
| objectMetadataItem.isCustom || | ||
| SEARCHABLE_STANDARD_OBJECTS_NAMES_PLURAL.includes( | ||
| objectMetadataItem.namePlural, | ||
| ) | ||
| ); |
There was a problem hiding this comment.
style: Function looks correct, but consider adding a comment explaining the logic behind determining searchability
| export const formatSearchResults = ( | ||
| searchResults: MultiObjectRecordQueryResult | undefined, | ||
| ): MultiObjectRecordQueryResult => { | ||
| if (!searchResults) { | ||
| return {}; | ||
| } | ||
|
|
||
| return Object.entries(searchResults).reduce((acc, [key, value]) => { | ||
| let newKey = key.replace(/^search/, ''); | ||
| newKey = newKey.charAt(0).toLowerCase() + newKey.slice(1); | ||
| acc[newKey] = value; | ||
| return acc; | ||
| }, {} as MultiObjectRecordQueryResult); | ||
| }; |
There was a problem hiding this comment.
style: consider moving this utility function to a separate file for better code organization
| const { | ||
| loading: selectedAndMatchesSearchFilterObjectRecordsLoading, | ||
| data: selectedAndMatchesSearchFilterObjectRecordsQueryResult, | ||
| } = useQuery<MultiObjectRecordQueryResult>( | ||
| multiSelectQueryForSelectedIds ?? EMPTY_QUERY, | ||
| multiSelectSearchQueryForSelectedIds ?? EMPTY_QUERY, |
There was a problem hiding this comment.
logic: multiSelectSearchQueryForSelectedIds is used here, but multiSelectQueryForSelectedIds is used in the skip condition. This inconsistency could lead to unexpected behavior
| ...selectedAndMatchesSearchFilterTextFilterPerMetadataItem, | ||
| ...orderByFieldPerMetadataItem, | ||
| ...limitPerMetadataItem, | ||
| }, | ||
| skip: !isDefined(multiSelectQueryForSelectedIds), |
There was a problem hiding this comment.
logic: ensure multiSelectQueryForSelectedIds is still needed, or if it should be replaced with multiSelectSearchQueryForSelectedIds
...ject-record/relation-picker/hooks/useMultiObjectSearchMatchesSearchFilterAndToSelectQuery.ts
Show resolved
Hide resolved
| 'people', | ||
| 'opportunities', | ||
| ]; | ||
| export const isObjectMetadataItemSearchable = ( |
There was a problem hiding this comment.
I think this name is confusing because technically 'notes' are now searchable as well. If you want this util to be specific to multi objects search maybe we can find a better name? If it could work for notes then let's add it!
Fixes #3298.
We still have some existing glitches in the picker yet to fix.