feat: record board component state refactor #8779

magrinj · 2024-11-27T14:48:55Z

This PR is migrating the recoil component state from v1 to v2 for board.
It also now share some states and logics between board and table, further can be done later.
Lastly this PR fix an issue since the PR #8613 that was treating no-value as a normal record-group.

charlesBochet · 2024-11-27T16:13:17Z

packages/twenty-front/src/modules/object-record/record-board/hooks/useRecordBoardSelection.ts

+      recordBoardId,
+    );
+
+  const recordBoardSelectedRecordIdsSelector =


I think we should also extract selection at recordIndex level too (but that's another story!)

Yes I think too, I didn't make it here because the PR was big enough 😅

charlesBochet · 2024-11-27T16:14:25Z

packages/twenty-front/src/modules/object-record/record-board/hooks/useSetRecordIdsForColumn.ts

+import { isDeeplyEqual } from '~/utils/isDeeplyEqual';
+import { isDefined } from '~/utils/isDefined';
+
+export const useSetRecordIdsForColumn = (recordBoardId?: string) => {


looks similar to useSetRecordBoardRecordIds?

naming is not consistent: useSetRecordIdsForColumn + do we need to do it twice?

It's similar but not exactly doing the same, I didn't take time to figure out why we need both of them, but they're used

ok, could you add a TODO?

charlesBochet · 2024-11-27T16:15:13Z

.../src/modules/object-record/record-board/record-board-column/components/RecordBoardColumn.tsx

  );

-  const recordIds = useRecoilValue(
-    recordIdsByColumnIdFamilyState(recordBoardColumnId),
+  const recordRowIdsByGroup = useRecoilComponentFamilyValueV2(


should be recordCardIdsByGroup here?

just for me to double check: what is rowIds vs recordIds? is it actually recordIds?

to me rowIds are only useful to navigate between records using the keyboard, I'm a bit surprise we have this on Kanban

Actually it's record ids (uuids), I'm a bit confused too with the naming as before states was names with rowIds, but I think we should use recordIds everywhere ?

okay, I think we should rename them all to recordIds, it would be way clearer

greptile-apps

PR Summary

This PR implements a significant refactoring of the record board component state management, moving from hook-based to component-scoped Recoil state management.

Replaced useRecordBoardStates and related hooks with direct component-scoped Recoil states using useRecoilComponentValueV2 and useRecoilComponentFamilyStateV2
Migrated from table-specific states to more generic record index states (e.g., tableAllRowIdsComponentState -> recordIndexAllRowIdsComponentState)
Introduced RecordBoardComponentInstanceContext for better state isolation and management
Added new utility functions like recordGroupDefinitionToViewGroup and sortedInsert to improve code organization
Improved handling of 'No Value' groups in view group mapping with proper UUID generation and positioning

_{💡 (4/5) You can add custom instructions or style guidelines for the bot here!}

_{95 file(s) reviewed, 41 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-11-27T15:20:29Z

...odules/object-record/object-options-dropdown/components/ObjectOptionsDropdownMenuContent.tsx

@@ -113,7 +114,7 @@ export const ObjectOptionsDropdownMenuContent = () => {
            onClick={() => onContentChange('recordGroups')}
            LeftIcon={IconLayoutList}
            text="Group by"
-            contextualText={viewGroupFieldMetadataItem?.label}
+            contextualText={recordGroupFieldMetadata?.label}


style: optional chaining here could result in 'undefined' being displayed - consider adding a default value

greptile-apps · 2024-11-27T15:21:54Z

...ct-record/object-options-dropdown/components/ObjectOptionsDropdownRecordGroupSortContent.tsx

  useEffect(() => {
    if (
      currentContentId === 'hiddenRecordGroups' &&
-      hiddenRecordGroups.length === 0
+      hiddenRecordGroupIds.length === 0
    ) {
      onContentChange('recordGroups');
    }
-  }, [hiddenRecordGroups, currentContentId, onContentChange]);
+  }, [hiddenRecordGroupIds, currentContentId, onContentChange]);


style: this useEffect could potentially be moved to a shared hook since the same pattern appears in multiple dropdown content components

greptile-apps · 2024-11-27T15:23:50Z

packages/twenty-front/src/modules/object-record/record-board/components/RecordBoard.tsx

      onColumnsChange={() => {}}
      onFieldsChange={() => {}}


logic: Empty callback functions should be implemented or removed if not needed

greptile-apps · 2024-11-27T15:24:17Z

packages/twenty-front/src/modules/object-record/record-board/components/RecordBoardHeader.tsx

-        <RecordBoardColumnHeaderWrapper columnId={columnId} key={columnId} />
+      {visibleRecordGroupIds.map((recordGroupId) => (
+        <RecordBoardColumnHeaderWrapper
+          columnId={recordGroupId}


style: columnId prop is being passed a recordGroupId value - consider renaming the prop to match the new data structure

greptile-apps · 2024-11-27T15:24:48Z

...s/twenty-front/src/modules/object-record/record-board/hooks/internal/useRecordBoardStates.ts

logic: This file managed critical state for record board functionality including filters, sorts, field definitions, and selection states. Ensure all this functionality is preserved in the new implementation.

greptile-apps · 2024-11-27T16:13:40Z

...ront/src/modules/ui/utilities/state/component-state/hooks/useRecoilComponentFamilyStateV2.ts

+  if (!componentInstanceContext) {
+    throw new Error(
+      `Instance context for key "${componentState.key}" is not defined`,
+    );
+  }


style: error message should include more context about how to properly initialize the context

greptile-apps · 2024-11-27T16:14:25Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewGroups.ts

+        const existingField = currentViewGroups.find(
+          (currentViewGroup) =>
+            currentViewGroup.fieldValue === viewGroupToSave.fieldValue,
+        );


style: variable name 'existingField' is inconsistent with type 'ViewGroup', should be 'existingViewGroup' for clarity

greptile-apps · 2024-11-27T16:14:26Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewGroups.ts

+        await updateViewGroupRecords([
+          { ...viewGroupToSave, id: existingField.id },
+        ]);
+      },


logic: saveViewGroup doesn't handle creation of new view groups, unlike saveViewGroups. This inconsistency could lead to bugs

greptile-apps · 2024-11-27T16:15:27Z

packages/twenty-front/src/modules/views/utils/mapViewGroupsToRecordGroupDefinitions.ts

+        viewGroup?.position ??
+        recordGroupDefinitionsFromViewGroups
+          .map((option) => option.position)
+          .reduce((a, b) => Math.max(a, b), 0) + 1,


logic: potential error if recordGroupDefinitionsFromViewGroups is empty - reduce will throw

greptile-apps · 2024-11-27T16:16:11Z

packages/twenty-front/src/modules/views/utils/recordGroupDefinitionToViewGroup.ts

+    fieldMetadataId: recordGroup.fieldMetadataId,
+    position: recordGroup.position,
+    isVisible: recordGroup.isVisible ?? true,
+    fieldValue: recordGroup.value ?? '',


logic: fieldValue should be nullable to match potential null values from the backend

charlesBochet · 2024-11-28T10:34:45Z

...twenty-front/src/modules/object-record/record-group/hooks/useCurrentRecordGroupDefinition.ts


-export const useCurrentRecordGroupDefinition = (recordTableId?: string) => {
+export const useCurrentRecordGroupDefinition = () => {


why "Current here"

It's because we're getting the RecordGroup based on the current recordGroupId present in the context, but we can change the naming if you want

charlesBochet · 2024-11-28T10:37:13Z

...enty-front/src/modules/object-record/record-group/states/recordGroupDefinitionFamilyState.ts

+import { RecordGroupDefinition } from '@/object-record/record-group/types/RecordGroupDefinition';
+import { atomFamily } from 'recoil';
+
+export const recordGroupDefinitionFamilyState = atomFamily<


what is the recordGroupDefinition used for? I feel the "Definition" pattern is not useful anymore and could be replaced by utils

RecordGroupDefinition is almost a mapping of view-groups, at first it was added because of the NoValue group that is not stored in the backend side. But as we now wan't it to act as a normal RecordGroup this one is stored in the backend too.
But I think we should keep it now for consistency in the code.

ok, let's do a follow up

charlesBochet · 2024-11-28T10:37:56Z

...ront/src/modules/object-record/record-group/states/recordGroupFieldMetadataComponentState.ts

+>({
+  key: 'recordGroupFieldMetadataComponentState',
+  defaultValue: undefined,
+  componentInstanceContext: ViewComponentInstanceContext,


should not use ViewComponentInstanceContext in RecordGroups

The context war haha, actually mixing context between different modules can be tricky because some selectors or are mixing some states from different modules.
It's a bit due to the refactor, but also due to the structure of record-group, maybe we should move it inside record-index.
Mixing states in selectors can actually work as the instanceId between context is often the same, but it's not a good option.

yes, ok, let's do that in a folllow up

charlesBochet · 2024-11-28T10:38:14Z

...modules/object-record/record-group/states/selectors/hiddenRecordGroupIdsComponentSelector.ts

+  string[]
+>({
+  key: 'hiddenRecordGroupIdsComponentSelector',
+  componentInstanceContext: ViewComponentInstanceContext,


same everywhere :)

charlesBochet · 2024-11-28T10:40:24Z

...dules/object-record/record-group/states/selectors/recordGroupDefinitionsComponentSelector.ts

+import { ViewComponentInstanceContext } from '@/views/states/contexts/ViewComponentInstanceContext';
+import { isDefined } from '~/utils/isDefined';
+
+export const recordGroupDefinitionsComponentSelector =


charlesBochet · 2024-11-28T10:42:25Z

packages/twenty-front/src/modules/object-record/record-group/utils/sortedInsert.ts

@@ -0,0 +1,20 @@
+export const sortedInsert = <T>(


I'm not a big fan of this function which is doing two things: sorting + insert. Not sure we need a combined logic here.
why not just sorting where needed and inserting when needed?

I make this util to avoid looping too many times on the recordGroupIds for performance purpose.
By using it it's reduced to 2 loop, and I think that can help with big tables.

ok, let's give it a local naming then: recordGroupSortIndex so people don't start using it

charlesBochet · 2024-11-28T10:44:57Z

...ty-front/src/modules/object-record/record-index/states/recordIndexAllRowIdsComponentState.ts

+>({
+  key: 'recordIndexAllRowIdsComponentState',
+  defaultValue: [],
+  componentInstanceContext: ViewComponentInstanceContext,


charlesBochet · 2024-11-28T10:49:10Z

packages/twenty-front/src/modules/views/hooks/useSaveCurrentViewGroups.ts

@@ -20,6 +20,56 @@ export const useSaveCurrentViewGroups = (viewBarComponentId?: string) => {
    viewBarComponentId,
  );

+  const saveViewGroup = useRecoilCallback(
+    ({ snapshot }) =>
+      async (viewGroupToSave: ViewGroup) => {


views hooks should expose RecordGroups in its API and internally convert it to view Group, this way everybody is taking dependency on RecordGroup (types) and ViewGroups is only in ViewBar

I'll take a look on that, maybe in another PR ?

yes, please discuss with Lucas too on this

charlesBochet

Bravo!

feat: record board component state refactor

14ebef5

magrinj force-pushed the feat/record-board-component-state-refactor branch from 940d4b4 to 14ebef5 Compare November 27, 2024 15:04

magrinj added 2 commits November 27, 2024 16:05

fix: rebase

e40e59d

fix: no value record group

5fb1e59

magrinj marked this pull request as ready for review November 27, 2024 15:19

magrinj requested review from lucasbordeau and charlesBochet November 27, 2024 15:19

magrinj added the -PR: awaiting review label Nov 27, 2024

fix: lint

65b0fd4

charlesBochet self-assigned this Nov 27, 2024

charlesBochet reviewed Nov 27, 2024

View reviewed changes

fix: miss to remove unused file

69e56eb

charlesBochet reviewed Nov 27, 2024

View reviewed changes

greptile-apps bot reviewed Nov 27, 2024

View reviewed changes

fix: hide empty record groups

25c8312

charlesBochet reviewed Nov 28, 2024

View reviewed changes

charlesBochet approved these changes Nov 28, 2024

View reviewed changes

charlesBochet merged commit 812ed6e into main Nov 28, 2024
13 of 16 checks passed

charlesBochet deleted the feat/record-board-component-state-refactor branch November 28, 2024 12:44


		export const useCurrentRecordGroupDefinition = (recordTableId?: string) => {
		export const useCurrentRecordGroupDefinition = () => {

feat: record board component state refactor #8779

feat: record board component state refactor #8779

Conversation

magrinj commented Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magrinj Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlesBochet left a comment

Choose a reason for hiding this comment

magrinj commented Nov 27, 2024 •

edited

Loading

charlesBochet Nov 28, 2024 •

edited

Loading

magrinj Nov 28, 2024 •

edited

Loading