Implement hide empty groups for grouped table view#16494
Merged
FelixMalfait merged 9 commits intomainfrom Jan 9, 2026
Merged
Conversation
- Replaced the `RecordTableRecordGroupBodyContextProvider` with a new `RecordTableRecordGroupWrapper` component to streamline the rendering of record groups. - The new wrapper component encapsulates the logic for handling empty record groups and integrates the necessary context providers and droppable areas. - This change improves code organization and enhances the readability of the `RecordTableRecordGroupsBody` component.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableRecordGroupsBody.tsx">
<violation number="1" location="packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableRecordGroupsBody.tsx:37">
P1: When the first record group (`index === 0`) is empty and `shouldHideEmptyRecordGroups` is true, `RecordTableCellPortals` will never render because the wrapper returns `null` before reaching the portal rendering logic. This would break table cell interactions (hover states, focus, edit mode, arrow key navigation). Consider moving `RecordTableCellPortals` outside the wrapper or checking if it's the first *visible* group rather than first group by index.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
...ules/object-record/record-table/record-table-body/components/RecordTableRecordGroupsBody.tsx
Outdated
Show resolved
Hide resolved
Contributor
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:6004 This environment will automatically shut down when the PR is closed or after 5 hours. |
- Removed the `RecordTableRecordGroupWrapper` component and integrated its logic directly into `RecordTableRecordGroupsBody`. - Introduced filtering for empty record groups based on the `shouldHideEmptyRecordGroups` state. - Enhanced the rendering logic to improve code organization and maintainability. - Updated the component structure to utilize new context providers and droppable areas for better functionality.
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableRecordGroupsBody.tsx">
<violation number="1" location="packages/twenty-front/src/modules/object-record/record-table/record-table-body/components/RecordTableRecordGroupsBody.tsx:46">
P2: Using `useRecoilCallback` with immediate invocation during render may cause reactivity issues. The component reads `recordIdsByGroup` from snapshot without subscribing to it, so it won't re-render when group contents change (e.g., when a record is added to an empty group or all records are removed from a group). Consider using a selector for this derived state, or subscribing to the necessary state values directly.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
...ules/object-record/record-table/record-table-body/components/RecordTableRecordGroupsBody.tsx
Outdated
Show resolved
Hide resolved
- Introduced a new selector to filter visible record group IDs based on the presence of record IDs, enhancing the handling of empty record groups. - Updated `RecordTableRecordGroupsBody` to utilize the new selector, streamlining the filtering logic and improving code organization. - Removed redundant state management related to empty record groups, simplifying the component's structure.
ijreilly
approved these changes
Dec 15, 2025
| import { ViewComponentInstanceContext } from '@/views/states/contexts/ViewComponentInstanceContext'; | ||
| import { type ViewType } from '@/views/types/ViewType'; | ||
|
|
||
| export const filteredVisibleRecordGroupIdsComponentFamilySelector = |
Member
There was a problem hiding this comment.
neat: creating a selector is a bit overkill
...ules/object-record/record-table/record-table-body/components/RecordTableRecordGroupsBody.tsx
Outdated
Show resolved
Hide resolved
| export const filteredVisibleRecordGroupIdsComponentFamilySelector = | ||
| createComponentFamilySelector<RecordGroupDefinition['id'][], ViewType>({ | ||
| key: 'filteredVisibleRecordGroupIdsComponentFamilySelector', | ||
| componentInstanceContext: ViewComponentInstanceContext, |
Member
There was a problem hiding this comment.
does not feel write to use ViewComponentInstanceContext for RecordIndex but it seems to be used everywhere in the current code so LGTM
charlesBochet
previously requested changes
Dec 15, 2025
Member
charlesBochet
left a comment
charlesBochet
left a comment
There was a problem hiding this comment.
Left some comments actually @abdulrahmancodes :)
@ijreilly FYI
- Updated `RecordBoardColumns`, `RecordBoardHeader`, and `RecordBoardColumn` components to utilize the new `filteredVisibleRecordGroupIdsComponentFamilySelector`, enhancing the filtering logic for visible record groups. - Removed unused state management related to empty record groups, simplifying the component structure and improving maintainability.
FelixMalfait
approved these changes
Jan 8, 2026
FelixMalfait
approved these changes
Jan 8, 2026
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Hey @FelixMalfait! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
Contributor
|
Thanks @abdulrahmancodes for your contribution! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.