fix: record group issues #8854
fix: record group issues #8854
Conversation
magrinj
force-pushed
the
fix/record-group-issues
branch
from
6200f28 to
7a8fa12
Compare
![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 addresses record group functionality issues by deprecating kanbanFieldMetadataId and implementing proper group field handling across frontend and backend.
- Added transaction support in
field-metadata-related-records.service.tsfor data consistency - Improved view group cleanup in
useHandleRecordGroupField.tswhen switching group-by fields - Added skip logic for views without groups in
field-metadata-related-records.service.ts - Fixed Select field metadata option handling to prevent view group breakage
- Marked
kanbanFieldMetadataIdas deprecated across types with proper JSDoc comments
5 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -18,6 +18,9 @@ export type View = { | |||
| viewFilters: ViewFilter[]; | |||
| viewFilterGroups?: ViewFilterGroup[]; | |||
| viewSorts: ViewSort[]; | |||
| /** | |||
| * @deprecated Use `viewGroups.fieldMetadataId` instead. | |||
| */ | |||
| kanbanFieldMetadataId: string; | |||
There was a problem hiding this comment.
style: Consider making kanbanFieldMetadataId optional since it's deprecated
| @@ -84,6 +84,9 @@ export class ViewWorkspaceEntity extends BaseWorkspaceEntity { | |||
| label: 'kanbanfieldMetadataId', | |||
There was a problem hiding this comment.
style: label should be 'Kanban Field Metadata Id' to match standard casing
| const valuesToDelete = deleted.map((option) => option.value); | ||
|
|
||
| await viewGroupRepository.delete({ | ||
| fieldMetadataId: newFieldMetadata.id, | ||
| fieldValue: In(valuesToDelete), | ||
| }); | ||
| await viewGroupRepository.delete( | ||
| { | ||
| fieldMetadataId: newFieldMetadata.id, | ||
| fieldValue: In(valuesToDelete), | ||
| }, | ||
| transactionManager, | ||
| ); |
There was a problem hiding this comment.
style: check if valuesToDelete is empty before performing delete operation to avoid unnecessary database calls
| const viewGroupsToDelete = view.viewGroups.filter( | ||
| (group) => group.fieldMetadataId !== fieldMetadataItem.id, | ||
| ); |
There was a problem hiding this comment.
logic: this filter removes all groups from other fields, which means switching back to a previous field will lose any customizations made to those groups
| if (viewGroupsToCreate.length > 0) { | ||
| await createViewGroupRecords(viewGroupsToCreate, view); | ||
| } | ||
|
|
||
| if (viewGroupsToDelete.length > 0) { | ||
| await deleteViewGroupRecords(viewGroupsToDelete); | ||
| } |
There was a problem hiding this comment.
style: the order of operations here could cause a brief visual flicker - consider deleting old groups before creating new ones
|
LGTM! |
This PR is fixing the following issues with record groups: - [x] [Backend] - Only update view groups when a field is edited if this one already has view groups - [x] [Backend] - Editing a Select field metadata option brake view groups - [x] [Frontend] - Changing the group by field from one to another brake record group and doesn't remove the previous ones - [x] [Frontend & Backend] - Mark `kanbanFieldMetadataId` as deprecated in favour of `viewGroups.fieldMetadataId` Also the following has been checked: - [x] Properly displayed a table with only one view groups - [x] Properly displayed a table without view groups
This PR is fixing the following issues with record groups:
kanbanFieldMetadataIdas deprecated in favour ofviewGroups.fieldMetadataIdAlso the following has been checked: