feat: default record group table #8397

magrinj · 2024-11-08T09:41:47Z

This PR is preparing states to we'll be able to handle view groups correctly with table data.
RowIds are now stores in 2 component states, one storing ids by view group and another storing all the rowIds.
We're doing that because some other state like focus, or select must not be scoped by view group id.

github-actions · 2024-11-08T09:42:45Z

TODOs/FIXMEs:

// TODO: Hack to store all ids in the same order as the record group definitions: packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useSetRecordTableData.ts

Generated by 🚫 dangerJS against a79f9ab

greptile-apps

PR Summary

This PR introduces record group functionality to the table view, enabling better organization and management of records. The changes focus on state management and data structure improvements.

Added RecordGroupContextProvider in /packages/twenty-front/src/modules/object-record/record-group/components/RecordGroupContextProvider.tsx to manage record groups and provide group context to child components
Refactored row identification from numeric indices to record IDs in /packages/twenty-front/src/modules/object-record/record-table/record-table-cell/hooks/useSelectedTableCellEditMode.ts for more stable cell referencing
Introduced dual state management for row IDs in /packages/twenty-front/src/modules/object-record/record-table/hooks/internal/useSetRecordTableData.ts - one for group-specific IDs and another for all row IDs
Added group-based filtering in /packages/twenty-front/src/modules/object-record/record-index/hooks/useLoadRecordIndexTable.ts to fetch records based on current record group definition
Implemented record group reordering functionality in /packages/twenty-front/src/modules/object-record/record-group/hooks/useRecordGroupReorder.ts with drag-and-drop support

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

greptile-apps · 2024-11-08T09:44:10Z

...wenty-front/src/modules/object-record/record-group/components/RecordGroupContextProvider.tsx

+      {visibleRecordGroups.map((recordGroup) => (
+        <RecordGroupContext.Provider
+          key={recordGroup.id}
+          value={{ recordGroupId: recordGroup.id }}
+        >
+          {children}
+        </RecordGroupContext.Provider>
+      ))}


logic: Rendering children multiple times (once per group) could cause performance issues with large child trees. Consider restructuring to avoid duplicating the entire child component tree for each group.

greptile-apps · 2024-11-08T09:44:10Z

...wenty-front/src/modules/object-record/record-group/components/RecordGroupContextProvider.tsx

+
+  if (visibleRecordGroups.length === 0) {
+    return (
+      <RecordGroupContext.Provider value={{ recordGroupId: 'default' }}>


style: Consider using recordGroupDefaultId constant from RecordGroupDefinition.ts instead of hardcoding 'default'

greptile-apps · 2024-11-08T09:44:11Z

...wenty-front/src/modules/object-record/record-group/components/RecordGroupContextProvider.tsx

+  const { visibleRecordGroups } = useRecordGroups({
+    objectMetadataNameSingular,
+  });


style: Destructuring only visibleRecordGroups but not handling loading states could cause flash of incorrect content

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

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

greptile-apps · 2024-11-08T09:54:41Z

packages/twenty-front/src/modules/object-record/record-table/hooks/useRecordTableMoveFocus.ts

        const softFocusPosition = getSnapshotValue(
          snapshot,
          softFocusPositionState,
        );

-        let newRowNumber = softFocusPosition.row - 1;
+        const currentRowIndex = allRowIds.indexOf(softFocusPosition.recordId);


logic: indexOf could return -1 if recordId not found in allRowIds, causing focus issues

greptile-apps · 2024-11-08T09:54:41Z

packages/twenty-front/src/modules/object-record/record-table/hooks/useRecordTableMoveFocus.ts

+        const allRowIds = getSnapshotValue(snapshot, tableAllRowIdsState);
        const softFocusPosition = getSnapshotValue(
          snapshot,
          softFocusPositionState,
        );


style: check if allRowIds is empty array before proceeding to avoid undefined access

greptile-apps · 2024-11-08T09:54:43Z

packages/twenty-front/src/modules/object-record/record-table/hooks/useRecordTableMoveFocus.ts

      setSoftFocusPosition,
    ],
  );

  const moveLeft = useRecoilCallback(
    ({ snapshot }) =>
      () => {
+        const rowIds = getSnapshotValue(snapshot, tableAllRowIdsState);


style: variable name 'rowIds' differs from 'allRowIds' used elsewhere - should be consistent

greptile-apps · 2024-11-08T09:56:10Z

...rc/modules/object-record/record-table/record-table-body/components/RecordTableBodyEffect.tsx

@@ -22,6 +23,8 @@ export const RecordTableBodyEffect = () => {

  const [hasInitializedScroll, setHasInitiazedScroll] = useState(false);


syntax: 'setHasInitiazedScroll' contains a typo in 'Initiazed'

greptile-apps · 2024-11-08T09:59:04Z

...-record/record-table/record-table-cell/hooks/__tests__/useSelectedTableCellEditMode.test.tsx

 describe('useSelectedTableCellEditMode', () => {
  it('should have property setSelectedTableCellEditMode', async () => {


style: test name 'should have property setSelectedTableCellEditMode' does not match what is actually being tested - the test verifies the behavior of the function, not just its existence

lucasbordeau · 2024-11-12T13:44:38Z

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

@@ -30,7 +30,7 @@ export const useRecordGroupActions = () => {
  });

  const { viewGroupFieldMetadataItem } = useRecordGroups({
-    objectNameSingular,
+    objectMetadataNameSingular: objectNameSingular,


We use objectNameSingular everywhere as a convention

lucasbordeau · 2024-11-12T13:56:30Z

packages/twenty-front/src/modules/object-record/record-group/types/RecordGroupDefinition.ts

+
+export type RecordGroupDefinitionId =
+  | RecordGroupDefinition['id']
+  | typeof recordGroupDefaultId;


I think we can find a better way to encode the "no record group" case, the most indicated way would be to use a different part of the tree when there is no record group, while re-using common components and logic.

Instead of checking if id === "default", we could have a recoil state / selector hasRecordGroups, and use this state higher up in the hierarchy to decide between for example : RecordTableBodyWithoutGroupEffect and recordGroup.map( => RecordTableBodyGroupEffect.

So general rule of thumb : let's try to create two parts of the tree, where it's needed, for the no group case and the multiple groups case, and have it very visible and high in the tree with a clear state that indicates what we mean, because those part of the codebase are very sensitive to technical debt and we should double the amount of effort to make it crystal clear.

Rule of thumb : it's easy to create components and states to do explicit conditional rendering and then re-use common logic inside them, so let's abuse it.

I'm pointing to an article about that, which @Devessier also shared recently and that is very on point here : https://kentcdodds.com/blog/aha-programming

It's better to have some duplicated code that we don't know how to factorize yet, but that is very clear, simple and explicit, instead of the opposite.

Thanks for mentioning @lucasbordeau! Yeah, knowing when to create abstractions is a tricky topic.

lucasbordeau · 2024-11-12T14:10:43Z

packages/twenty-front/src/modules/object-record/record-table/components/RecordTable.tsx

@@ -82,7 +83,11 @@ export const RecordTable = ({
        recordTableId={recordTableId}
        viewBarId={viewBarId}
      >
-        <RecordTableBodyEffect />
+        <RecordGroupContextProvider


This pattern is never used in the codebase (and shouldn't if that's the case), we shouldn't abstract a .map in a component that will loop on its children. The preferred way is to be explicit about the fact that a component is displaying multiple items. Children shouldn't be transcluded like a parameter. Instead we should use a component like RecordTableGroupBodyEffects, that will loop through what need to be displayed as children.

Component in the codebase can and should also be used to indicate meaning more than pure functionality, this way our JSX tree is very explicit about what is done.

For exemple here it could be :

RecordTableGroupBodyEffects

RecordTableBodyEffect

And we could use conditional rendering between the two at this level.

Another problem here is that the ContextProvider suffix should be used only to abstract some logic that will be put into the Context + the Context.Provider wrapping.

Here it hides a conditional rendering + a .map which should be made explicit at that level of the tree.

Also I'm not sure that we should put a RecordGroupContext if there's no actual record group, that would not be explicit and meaningful, and could confuse further feature development.

Thus having the tree splitting in two here, or even higher RecordTableWithoutGroup and RecordTableWithGroups (maybe too high but worth considering) ?

Also it's obvious after reading this PR that we should use a recoil state to know explicitly if we're in the case of having record groups or not, instead of encoding it in different places.

Trying to mix the no group case into the multiple group case with magic ids and ifs will create technical debt in the long run IMHO.

I'm not sure about how it will look like exactly though, but you get the idea here.

lucasbordeau · 2024-11-12T14:20:17Z

.../twenty-front/src/modules/object-record/record-table/hooks/internal/useSetRecordTableData.ts

+          if (visibleRecordGroupDefinitions.length !== 0) {
+            // TODO: Hack to store all ids in the same order as the record group definitions
+            // Should be replaced by something more efficient
+            const allRowIds: string[] = [];


Works for me

lucasbordeau · 2024-11-12T14:21:03Z

packages/twenty-front/src/modules/object-record/record-table/hooks/useRecordTableMoveFocus.ts

That works for me, using the record id instead of numbers is a good way to go here.

magrinj added 6 commits November 4, 2024 18:13

feat: wip

4c14c1b

Merge branch 'main' into feat/record-group-table

2ce6c18

feat: record group table wip

234eba4

feat: wip

9d4d1fa

feat: record table with default view group

d18f37b

fix: tests

6c75d8c

magrinj marked this pull request as ready for review November 8, 2024 09:43

magrinj requested review from charlesBochet and lucasbordeau November 8, 2024 09:43

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

greptile-apps bot reviewed Nov 8, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into feat/record-group-table

a79f9ab

magrinj force-pushed the feat/record-group-table branch from a906d45 to a79f9ab Compare November 12, 2024 11:13

lucasbordeau reviewed Nov 12, 2024

View reviewed changes

lucasbordeau added -PR: awaiting author and removed -PR: awaiting review labels Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: default record group table #8397

feat: default record group table #8397

magrinj commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Nov 8, 2024

greptile-apps bot Nov 8, 2024

greptile-apps bot Nov 8, 2024

greptile-apps bot Nov 8, 2024

greptile-apps bot Nov 8, 2024

greptile-apps bot Nov 8, 2024

greptile-apps bot Nov 8, 2024

greptile-apps bot Nov 8, 2024

lucasbordeau Nov 12, 2024

lucasbordeau Nov 12, 2024

Devessier Nov 12, 2024

lucasbordeau Nov 12, 2024 •

edited

Loading

lucasbordeau Nov 12, 2024

lucasbordeau Nov 12, 2024

		@@ -22,6 +23,8 @@ export const RecordTableBodyEffect = () => {

		const [hasInitializedScroll, setHasInitiazedScroll] = useState(false);

		describe('useSelectedTableCellEditMode', () => {
		it('should have property setSelectedTableCellEditMode', async () => {

feat: default record group table #8397

Are you sure you want to change the base?

feat: default record group table #8397

Conversation

magrinj commented Nov 8, 2024 • edited Loading

github-actions bot commented Nov 8, 2024 • edited Loading

TODOs/FIXMEs:

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

greptile-apps bot Nov 8, 2024

Choose a reason for hiding this comment

lucasbordeau Nov 12, 2024

Choose a reason for hiding this comment

lucasbordeau Nov 12, 2024

Choose a reason for hiding this comment

Devessier Nov 12, 2024

Choose a reason for hiding this comment

lucasbordeau Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

lucasbordeau Nov 12, 2024

Choose a reason for hiding this comment

lucasbordeau Nov 12, 2024

Choose a reason for hiding this comment

magrinj commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

lucasbordeau Nov 12, 2024 •

edited

Loading