Fix post merge revamp navigation bar #6297

lucasbordeau · 2024-07-17T08:32:00Z

Closes #6285

@charlesBochet Also added some more utils for our component state v2.

greptile-apps

PR Summary

Renamed RecordIndexRecordChip to RecordIdentifierChip across multiple files for consistency (RecordBoardCard.tsx, ChipFieldDisplay.tsx, RecordIndexRecordChip.tsx).
Replaced isFieldChipDisplay with isFieldIdentifierDisplay in FieldDisplay.tsx and getRecordChipGenerators.ts.
Updated URL construction logic in RecordIndexContainer.tsx to use buildShowPageURL utility function.
Refactored useRecordShowPagePagination for improved readability and maintainability.
Added RowHeight.ts to centralize ROW_HEIGHT constant, updated RecordTableBodyEffect.tsx to use it.

_{14 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps

PR Summary

(updates since last review)

Added useActiveFieldMetadataItems hook to filter active fields (packages/twenty-front/src/modules/object-metadata/hooks/useActiveFieldMetadataItems.ts).
Refactored RecordIndexContainer.tsx to use useHandleIndexIdentifierClick for navigation (packages/twenty-front/src/modules/object-record/record-index/components/RecordIndexContainer.tsx).
Introduced useHandleIndexIdentifierClick hook for handling index identifier clicks (packages/twenty-front/src/modules/object-record/record-index/hooks/useHandleIndexIdentifierClick.ts).
Simplified pagination logic in useRecordShowPagePagination.ts by removing redundant code (packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowPagePagination.ts).
Added useQueryVariablesFromActiveFieldsOfViewOrDefaultView hook to derive query variables (packages/twenty-front/src/modules/views/hooks/useQueryVariablesFromActiveFieldsOfViewOrDefaultView.ts).

_{7 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps

PR Summary

(updates since last review)

Deprecated useScrollRestoration in favor of useScrollToPosition (packages/twenty-front/src/hooks/useScrollRestoration.ts)
Added getCursorByRecordIdFromRecordConnection utility function for cursor and record ID mapping (packages/twenty-front/src/modules/object-record/cache/utils/getCursorByRecordIdFromRecordConnection.ts)
Enhanced useFetchMoreRecordsWithPagination with cursor state management (packages/twenty-front/src/modules/object-record/hooks/useFetchMoreRecordsWithPagination.ts)
Updated useFindManyRecords to include cursorsByRecordId property (packages/twenty-front/src/modules/object-record/hooks/useFindManyRecords.ts)
Added new state management files for handling cursors by record ID (packages/twenty-front/src/modules/object-record/record-table/states/cursorByRecordFamilyState.ts, packages/twenty-front/src/modules/object-record/record-table/states/tableCursorByRecordComponentStateV2.ts)

_{17 file(s) reviewed, 1 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-17T14:36:14Z

...t/src/modules/ui/utilities/state/component-state/hooks/useExtractedComponentFamilyStateV2.ts

+  if (!componentContext) {
+    throw new Error(
+      `Component context for key "${componentFamilyState.key}" is not defined`,
+    );


Style: Error message should be more descriptive for debugging.

greptile-apps

PR Summary

(updates since last review)

Renamed useComponentIdFromContext.ts to useScopeIdFromStateContext.ts and updated logic to retrieve scope ID using component state key (packages/twenty-front/src/modules/ui/utilities/state/component-state/hooks/useScopeIdFromStateContext.ts)

_{1 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

packages/twenty-front/src/modules/object-record/cache/utils/getRecordsFromRecordConnection.ts

charlesBochet · 2024-07-17T15:30:31Z

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

-            recordConnection: data?.[objectMetadataItem.namePlural],
-          })
-        : ([] as T[]),
+      getRecordsFromRecordConnection<T>({


I understand the usage, but I think we should catch it earlier is it possible to have data?.[objectMetadataItem.namePlural] undefined? shouldn't we throw/return earlier in this case instead

Since in React we try to avoid blocking initial renders where everything is loading and we should just allow the render flow to complete, I think it's preferable here to return [] since it will allow everything to show something and base itself on loading instead on trying to guess whether undefined means loading or error.

In this case there is already an error object that we can watch to handle errors, we shouldn't mix both.

...s/twenty-front/src/modules/object-record/record-index/hooks/useHandleIndexIdentifierClick.ts

...ages/twenty-front/src/modules/object-record/record-table/states/cursorByRecordFamilyState.ts

...y-front/src/modules/object-record/record-table/states/tableCursorByRecordComponentStateV2.ts

...t/src/modules/ui/utilities/state/component-state/hooks/useExtractedComponentFamilyStateV2.ts

charlesBochet · 2024-07-17T15:39:29Z

...nty-front/src/modules/ui/utilities/state/component-state/hooks/useScopeIdFromStateContext.ts

+import { ComponentFamilyState } from '@/ui/utilities/state/component-state/types/ComponentFamilyState';
+import { ComponentState } from '@/ui/utilities/state/component-state/types/ComponentState';
+
+export const useScopeIdFromStateContext = (


useComponentId!

It's not componentId it's scopeId, that's not the same, scopeId is prefixed with "-scope-id".

lucasbordeau · 2024-07-17T16:15:34Z

infer cursor from id instead of request to get it

lucasbordeau · 2024-07-17T16:15:45Z

Remove unused states

charlesBochet

Follow up:

infer cursor from recordId (in url)
introduce "navigation-store" module?

...wenty-front/src/modules/object-record/cache/utils/getCursorByRecordIdFromRecordConnection.ts

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

lucasbordeau added 3 commits July 16, 2024 18:00

Added proper view mock

733d995

WIP

d9e6c02

WIP

2511e95

lucasbordeau marked this pull request as draft July 17, 2024 08:32

greptile-apps bot reviewed Jul 17, 2024

View reviewed changes

refactored query variables from view

ce6a947

charlesBochet marked this pull request as ready for review July 17, 2024 12:00

greptile-apps bot reviewed Jul 17, 2024

View reviewed changes

lucasbordeau added 2 commits July 17, 2024 16:30

WIP

d9ae4b6

Fix

da887fe

greptile-apps bot reviewed Jul 17, 2024

View reviewed changes

charlesBochet approved these changes Jul 17, 2024

View reviewed changes

charlesBochet assigned lucasbordeau Jul 17, 2024

charlesBochet added the -PR: awaiting author label Jul 17, 2024

charlesBochet reviewed Jul 17, 2024

View reviewed changes

lucasbordeau added 3 commits July 18, 2024 16:46

Fixes from PR

4d5a9ce

Merge branch 'main' into fix/post-merge-revamp-navigation-bar

eb0edcd

Fix

ded2bd7

charlesBochet approved these changes Jul 19, 2024

View reviewed changes

...wenty-front/src/modules/object-record/cache/utils/getCursorByRecordIdFromRecordConnection.ts Outdated Show resolved Hide resolved

packages/twenty-front/src/modules/object-record/hooks/useFetchMoreRecordsWithPagination.ts Outdated Show resolved Hide resolved

Removed unused cursor by record id logic

7f37933

charlesBochet merged commit 1b0759e into main Jul 19, 2024
8 of 12 checks passed

charlesBochet deleted the fix/post-merge-revamp-navigation-bar branch July 19, 2024 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix post merge revamp navigation bar #6297

Fix post merge revamp navigation bar #6297

lucasbordeau commented Jul 17, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot Jul 17, 2024

greptile-apps bot left a comment

charlesBochet Jul 17, 2024

lucasbordeau Jul 18, 2024

charlesBochet Jul 17, 2024

lucasbordeau Jul 17, 2024

lucasbordeau commented Jul 17, 2024

lucasbordeau commented Jul 17, 2024

charlesBochet left a comment

Fix post merge revamp navigation bar #6297

Fix post merge revamp navigation bar #6297

Conversation

lucasbordeau commented Jul 17, 2024 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 17, 2024

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

charlesBochet Jul 17, 2024

Choose a reason for hiding this comment

lucasbordeau Jul 18, 2024

Choose a reason for hiding this comment

charlesBochet Jul 17, 2024

Choose a reason for hiding this comment

lucasbordeau Jul 17, 2024

Choose a reason for hiding this comment

lucasbordeau commented Jul 17, 2024

lucasbordeau commented Jul 17, 2024

charlesBochet left a comment

Choose a reason for hiding this comment

lucasbordeau commented Jul 17, 2024 •

edited

Loading