Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix re render view groups #9098

Merged
merged 5 commits into from
Dec 17, 2024
Merged

Fix re render view groups #9098

merged 5 commits into from
Dec 17, 2024

Conversation

charlesBochet
Copy link
Member

No description provided.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR optimizes record group visibility handling and reduces unnecessary re-renders in the view groups functionality.

  • Changed default value in recordIndexRecordGroupHideComponentState.ts to hide empty record groups by default
  • Simplified visibility state management in useRecordGroupVisibility.ts by removing complex state handling
  • Replaced dynamic UUID with static ID 20202020-c05f-46c9-ae1e-2b3c5c702049 for 'No Value' column in mapViewGroupsToRecordGroupDefinitions.ts
  • Switched from viewSorts to direct recordIndexSortsState usage in drag-and-drop context
  • Fixed redundant visibility checks in visibleRecordGroupIdsComponentSelector.ts

6 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -65,7 +64,7 @@ export const mapViewGroupsToRecordGroupDefinitions = ({
);

const noValueColumn = {
id: viewGroup?.id ?? v4(),
id: viewGroup?.id ?? '20202020-c05f-46c9-ae1e-2b3c5c702049',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Using a hardcoded UUID could cause collisions if multiple instances of this component exist simultaneously. Consider using a more unique identifier pattern or documenting this limitation.

Copy link
Collaborator

@ijreilly ijreilly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stunning...

@ijreilly ijreilly enabled auto-merge (squash) December 17, 2024 11:04
@@ -65,7 +64,7 @@ export const mapViewGroupsToRecordGroupDefinitions = ({
);

const noValueColumn = {
id: viewGroup?.id ?? v4(),
id: viewGroup?.id ?? '20202020-c05f-46c9-ae1e-2b3c5c702049',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to cause conflict issues on the backend side if a view group with this id already exist

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, as we introduce /backfill noValue groups in backend this will only be use for smooth migrations during 0.40 release. I think it's fine :)

@charlesBochet charlesBochet merged commit 4fe3250 into main Dec 17, 2024
22 checks passed
@charlesBochet charlesBochet deleted the fix-re-render-view-groups branch December 17, 2024 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants