fix: view group followup #9162
fix: view group followup #9162
Conversation
…ield selection" menu
![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 implements significant improvements to the group by and sort functionalities in Twenty's record management system. The changes focus on fixing UI/UX issues and enhancing performance.
- Switched from component selectors to family selectors for record groups with explicit view type parameters, improving state management granularity and performance
- Added confirmation modal for switching from alphabetical to manual sorting when reordering groups, with proper state handling via
recordGroupPendingDragEndReorderState - Implemented view-specific behavior for hiding empty groups using
recordIndexRecordGroupHideComponentFamilyState, with different defaults for Table (hidden) and Kanban (visible) views - Improved dropdown menu interactions by keeping menus open during sort changes and adding contextual text for group by and sort options
- Optimized view group persistence by replacing individual Apollo mutations with batch operations for better performance
32 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
| onClick={() => | ||
| isDefined(recordGroupFieldMetadata) | ||
| ? onContentChange('recordGroups') | ||
| : onContentChange('recordGroupFields') | ||
| } |
There was a problem hiding this comment.
style: Consider adding error handling in case recordGroupFieldMetadata is undefined but onContentChange fails
| import { atom } from 'recoil'; | ||
|
|
||
| export const recordGroupPendingDragEndReorderState = | ||
| atom<Parameters<OnDragEndResponder> | null>({ |
There was a problem hiding this comment.
style: Consider adding a more descriptive type alias instead of using Parameters directly, to improve code readability and maintainability
|
|
||
| export const recordGroupPendingDragEndReorderState = | ||
| atom<Parameters<OnDragEndResponder> | null>({ | ||
| key: 'recordGroupPendingDragEndReorderState', |
There was a problem hiding this comment.
style: Consider adding a more descriptive key prefix like 'recordGroup/' to avoid potential key collisions with other atoms
| isOpen={isReorderConfirmationModalOpen} | ||
| setIsOpen={setIsReorderConfirmationModalOpen} | ||
| title="Group sorting" | ||
| subtitle={`Would you like to remove ${recordGroupSort} group sorting`} |
There was a problem hiding this comment.
syntax: Missing punctuation at end of subtitle text
| if (!pendingDragEndReorder) { | ||
| throw new Error('pendingDragEndReorder is not set'); | ||
| } |
There was a problem hiding this comment.
logic: Error thrown here could cause UI to break if pendingDragEndReorder is cleared by another action. Consider graceful fallback
| default: | ||
| return a.position - b.position; |
There was a problem hiding this comment.
style: default case should be explicit rather than falling through from Manual case, in case new sort types are added
| case RecordGroupSort.Alphabetical: | ||
| return a.title.localeCompare(b.title); | ||
| case RecordGroupSort.ReverseAlphabetical: | ||
| return b.title.localeCompare(a.title); |
There was a problem hiding this comment.
style: localeCompare should include options for case sensitivity and locale to ensure consistent sorting across all browsers
| return false; | ||
| } |
There was a problem hiding this comment.
style: Default case returns false but could potentially handle unknown ViewTypes differently. Consider if this is the desired behavior for future view types.
| const updateViewGroupRecords = useCallback( | ||
| async (viewGroupsToUpdate: ViewGroup[]) => { | ||
| if (!viewGroupsToUpdate.length) return; | ||
|
|
||
| const mutationPromises = viewGroupsToUpdate.map((viewGroup) => | ||
| apolloClient.mutate<{ updateViewGroup: ViewGroup }>({ | ||
| mutation: updateOneRecordMutation, | ||
| variables: { | ||
| idToUpdate: viewGroup.id, | ||
| input: { | ||
| isVisible: viewGroup.isVisible, | ||
| position: viewGroup.position, | ||
| }, | ||
| return viewGroupsToUpdate.map((viewGroup) => | ||
| updateOneRecord({ | ||
| idToUpdate: viewGroup.id, | ||
| updateOneRecordInput: { | ||
| isVisible: viewGroup.isVisible, | ||
| position: viewGroup.position, | ||
| }, | ||
| // Avoid cache being updated with stale data | ||
| fetchPolicy: 'no-cache', | ||
| }), | ||
| ); | ||
|
|
||
| const mutationResults = await Promise.all(mutationPromises); | ||
|
|
||
| // FixMe: Using triggerCreateRecordsOptimisticEffect is actaully causing multiple records to be created | ||
| mutationResults.forEach(({ data }) => { | ||
| const record = data?.['updateViewGroup']; | ||
|
|
||
| if (!record) return; | ||
|
|
||
| apolloClient.cache.modify({ | ||
| id: apolloClient.cache.identify({ | ||
| __typename: 'ViewGroup', | ||
| id: record.id, | ||
| }), | ||
| fields: { | ||
| isVisible: () => record.isVisible, | ||
| position: () => record.position, | ||
| }, | ||
| }); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
logic: updateViewGroupRecords returns an array of promises but doesn't await them, which could lead to race conditions or incomplete updates
| const deleteViewGroupRecords = useCallback( | ||
| async (viewGroupsToDelete: ViewGroup[]) => { | ||
| if (!viewGroupsToDelete.length) return; | ||
|
|
||
| const mutationPromises = viewGroupsToDelete.map((viewGroup) => | ||
| apolloClient.mutate<{ deleteViewGroup: ViewGroup }>({ | ||
| mutation: deleteOneRecordMutation, | ||
| variables: { | ||
| idToDelete: viewGroup.id, | ||
| }, | ||
| // Avoid cache being updated with stale data | ||
| fetchPolicy: 'no-cache', | ||
| }), | ||
| return destroyManyRecords( | ||
| viewGroupsToDelete.map((viewGroup) => viewGroup.id), | ||
| ); | ||
|
|
||
| const mutationResults = await Promise.all(mutationPromises); | ||
|
|
||
| mutationResults.forEach(({ data }) => { | ||
| const record = data?.['deleteViewGroup']; | ||
|
|
||
| if (!record) return; | ||
|
|
||
| apolloClient.cache.evict({ | ||
| id: apolloClient.cache.identify({ | ||
| __typename: 'ViewGroup', | ||
| id: record.id, | ||
| }), | ||
| }); | ||
| }); | ||
| }, | ||
| [apolloClient, deleteOneRecordMutation], | ||
| [destroyManyRecords], | ||
| ); |
There was a problem hiding this comment.
style: consider adding error handling for failed deletions since multiple records are being deleted in a batch
There was a problem hiding this comment.
Some remarks :)
1 - I observed a weird behaviour when creating a view group from an empty records page:
https://github.com/user-attachments/assets/26d4aa3f-6556-4bf4-8cd8-3638f1aaf719
2 - nit but in the dropdown on Sort, when opening the Ascending / Descending dropdown, the submenu is "merged" with the bigger dropdown, maybe not intended?
https://github.com/user-attachments/assets/56dacc61-a0df-498f-98a0-222e51b0841f
| /> | ||
| )} | ||
| {(viewType === ViewType.Kanban || isViewGroupEnabled) && | ||
| currentView?.key !== 'INDEX' && ( |
There was a problem hiding this comment.
Why do we have this change ? I can't see the group by option from the table view, had to remove it to be able to create a view group
There was a problem hiding this comment.
Oh I see, it is to disable the option from the main view right ?
I feel this may prevent users from discovering the feature. Instead I would create a new view when selecting group by. wdyt @Bonapara ?
| ); | ||
|
|
||
| const hiddenRecordGroupIds = useRecoilComponentValueV2( | ||
| hiddenRecordGroupIdsComponentSelector, | ||
| ); | ||
|
|
||
| const isDragableSortRecordGroup = useRecoilComponentValueV2( | ||
| recordIndexRecordGroupIsDraggableSortComponentSelector, | ||
| const hideEmptyRecordGroup = useRecoilComponentFamilyValueV2( |
There was a problem hiding this comment.
just a thought - are we sure we want to hide the empty groups by default? I would have done the opposite, otherwise it can feel like they are just missing (that was my impression)
@Bonapara
There was a problem hiding this comment.
@ijreilly It's only hidden by default on the table view
There was a problem hiding this comment.
Yes but it means you have to create a new view to access it. I was just pointing out that I wasnt sure that is what was intended, i will let thomas clarify !
There was a problem hiding this comment.
It was actually requested by @Bonapara 🙂
There was a problem hiding this comment.
I said that since Notion hides empty groups by default, I think it's a safe behavior that prevents clutter, but this can change in the future!
This PR fixes all followup that @Bonapara add on Discord.
Performance improvement:
https://github.com/user-attachments/assets/fd2acf66-0e56-45d0-8b2f-99c62e57d6f7
https://github.com/user-attachments/assets/80f1a2e1-9f77-4923-b85d-acb9cad96886