Integrate NavigationMenuItem with feature flag support #17268
+3,620
−52
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Introduced `FlatNavigationMenuItem` type to represent navigation menu items. - Created `FlatNavigationMenuItemMaps` type for mapping entities. - Added `FLAT_NAVIGATION_MENU_ITEM_EDITABLE_PROPERTIES` constant to define editable properties for flat navigation menu items.
- Created a new migration to add the `navigationMenuItem` table in the core schema. - Defined columns, primary key, and foreign key constraints. - Added necessary indexes for improved query performance.
- Introduced `navigationMenuItem` to `ALL_FLAT_ENTITY_PROPERTIES_TO_COMPARE_AND_STRINGIFY` for comparison and stringification. - Added `navigationMenuItem` relations to `ALL_METADATA_RELATIONS`. - Updated validation requirements for `navigationMenuItem` in `ALL_METADATA_REQUIRED_METADATA_FOR_VALIDATION`. - Defined actions for `navigationMenuItem` in `AllFlatEntityTypesByMetadataName`. - Created new action types for navigation menu item migrations.
- Introduced `FlatNavigationMenuItemModule` to manage navigation menu items. - Created `WorkspaceFlatNavigationMenuItemMapCacheService` for caching logic related to flat navigation menu items. - Added utility function `fromNavigationMenuItemEntityToFlatNavigationMenuItem` for transforming entities. - Updated workspace cache keys to include `flatNavigationMenuItemMaps` for improved cache management.
- Introduced `NavigationMenuItemException` for handling navigation menu item errors with user-friendly messages. - Created `WorkspaceMigrationNavigationMenuItemActionsBuilderService` for managing navigation menu item actions during migration. - Added `FlatNavigationMenuItemValidatorService` to validate creation, deletion, and updates of navigation menu items. - Implemented action handlers for creating, updating, and deleting navigation menu items in the migration runner. - Updated the workspace migration builder module to include navigation menu item services and validators.
…pport - Integrated `WorkspaceMigrationNavigationMenuItemActionsBuilderService` into the orchestrator for handling navigation menu item actions during migration. - Implemented validation and building logic for navigation menu items, including error handling and action reporting. - Updated the orchestrator to merge navigation menu item results into the overall migration report.
- Introduced `NavigationMenuItemModule` to encapsulate navigation menu item functionality. - Created service and resolver for managing navigation menu items, including creation, update, and deletion operations. - Added input and DTO types for navigation menu item operations. - Implemented utility functions for transforming navigation menu item data and handling exceptions. - Integrated navigation menu item support into the existing metadata engine structure.
- Updated `optimisticallyApplyCreateActionOnAllFlatEntityMaps`, `optimisticallyApplyDeleteActionOnAllFlatEntityMaps`, and `optimisticallyApplyUpdateActionOnAllFlatEntityMaps` utilities to include support for `navigationMenuItem`. - Enhanced the handling of navigation menu items during optimistic actions in the workspace migration process.
- Introduced `NavigationMenuItem` type to the GraphQL schema, defining its fields and types for better integration with navigation menu functionalities. - This addition supports the ongoing development of navigation menu item features and enhances the overall schema structure.
- Introduced mutations for creating, deleting, and updating navigation menu items, enhancing the GraphQL API for navigation functionalities. - Added corresponding query support to retrieve navigation menu items, improving data accessibility and management within the application. - Defined input types for navigation menu item operations to streamline data handling and validation.
- Included `NavigationMenuItemEntity` in the `ALL_METADATA_ENTITY_BY_METADATA_NAME` constant to support navigation menu functionalities. - This addition enhances the metadata structure for better integration with navigation features.
…em files - Updated TypeScript imports to use type-only imports for better clarity. - Enhanced formatting in various utility and service files related to navigation menu items for improved readability. - Adjusted method signatures to align with consistent formatting practices across the codebase. - Refactored migration file for `NavigationMenuItemEntity` to maintain consistent query formatting. These changes aim to enhance code maintainability and readability across the navigation menu item functionalities.
…ity file - Renamed the index for target record object metadata in both the migration file and the entity definition to improve clarity and consistency. - This change enhances the maintainability of the database schema and aligns with naming conventions.
- Introduced comprehensive integration tests for creating, updating, and deleting navigation menu items, covering both successful and failing scenarios. - Added utility functions for creating and deleting navigation menu items to streamline test setup. - Enhanced test coverage to ensure robust validation of input parameters and error handling for navigation menu item operations. - These additions improve the overall reliability and maintainability of the navigation menu item functionalities.
…rage - Modified the `favoriteFolderId` property in `UpdateNavigationMenuItemInput` to accept `null` as a valid value, improving flexibility in navigation menu item updates. - Updated integration tests to verify the behavior of setting `favoriteFolderId` to `null`, ensuring comprehensive coverage of this scenario. - Adjusted import statements for consistency and clarity across utility files related to navigation menu item operations.
- Adjusted the formatting of the `input` object in the failing navigation menu item update integration test for improved readability. - Reformatted the export statement in the update navigation menu item query factory utility file to enhance clarity and maintain consistent code style.
…lity - Adjusted the formatting of the error message in the `createNavigationMenuItem` utility for better readability and consistency with coding standards.
- Adjusted the import statements in the `failing-navigation-menu-item-update.integration-spec.ts` file for better organization and clarity. - Moved the `findManyObjectMetadata` import to maintain consistency with other utility imports and improve readability.
…stency - Adjusted the import statements in the `delete-navigation-menu-item.util.ts` file to improve organization and maintain consistency with other utility files. - This change enhances code readability and aligns with recent formatting updates across navigation menu item functionalities.
…hql.ts - Moved the Apollo import statement to improve organization. - Introduced a new type definition for QueryNavigationMenuItemArgs to enhance GraphQL query capabilities. - Minor formatting adjustments for consistency with existing code.
- Added a new snapshot for the `getMetadataRelatedMetadataNames` utility to include the `navigationMenuItem`. - Updated the snapshot for `sortMetadataNamesChildrenFirst` to reflect the addition of `navigationMenuItem` in the sorted output. - Minor adjustment to the snapshot comment for clarity.
- Changed validation decorators for `position` in both `create-navigation-menu-item.input.ts` and `update-navigation-menu-item.input.ts` from `@IsNumber()` to `@IsInt()` and added `@Min(0)` to ensure non-negative integer values. - Added new snapshot tests for failing scenarios in navigation menu item creation, deletion, and update to enhance validation coverage and error handling.
- Added checks to ensure that the `position` property for navigation menu items is a non-negative integer during both creation and update processes. - Updated error handling to return specific validation messages for invalid position inputs. - Adjusted snapshot tests to reflect changes in validation error responses for negative position scenarios.
- Modified the error message in the snapshot for the failing navigation menu item deletion test to accurately reflect the variable name in the GraphQL error response. - This change ensures consistency in error reporting and improves the clarity of test outputs.
…Ds in navigation menu item utility - Updated the utility function to normalize checks for `forWorkspaceMemberId` and `favoriteFolderId` by using null coalescing, improving the robustness of the existing item filtering logic. - This change enhances the accuracy of item comparisons when creating navigation menu items.
- Updated the filtering logic in the usePrefetchedFavoritesData hook to only include favorites with a null forWorkspaceMemberId, improving code clarity and reducing unnecessary checks.
… hook - Cleaned up the usePrefetchedFavoritesData hook by removing an unnecessary blank line, improving code readability.
…Data hook - Simplified the filtering logic to use the isDefined utility for checking userWorkspaceId, enhancing code clarity and reducing unnecessary checks.
- Introduced comprehensive unit tests for the computeNavigationMenuItemDisplayFields, getNavigationMenuItemSecondaryLabel, isLocationMatchingNavigationMenuItem, and sortNavigationMenuItems functions. - These tests cover various scenarios, including null inputs, expected outputs, and edge cases, ensuring robust validation of the navigation menu item utilities.
- Introduced unit tests for getObjectMetadataNamePluralFromViewId and sortFavorites functions, covering scenarios such as valid and invalid objectMetadataId, handling of favorites with and without viewId, and sorting by position. - These tests ensure robust validation of the favorites utilities, improving overall code reliability.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 22, 2026
Implements a new syncable `navigationMenuItem` entity in the core schema to replace the workspace `favorite` entity. ## Next Steps - Frontend integration ([separate PR](#17268)) - Data migration (separate PR)
Base automatically changed from
feat/migrate-favorite-to-navigation-menu-item
to
main
January 22, 2026 13:00
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
...orkspace-manager/twenty-standard-application/services/twenty-standard-application.service.ts
Show resolved
Hide resolved
Contributor
Greptile SummaryThis PR successfully integrates the new Key Implementation Highlights:
Areas of Concern:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant UI as UI Components
participant FeatureFlag as Feature Flag<br/>(IS_NAVIGATION_MENU_ITEM_ENABLED)
participant Hook as Hooks<br/>(useCreate/Update/Delete)
participant Prefetch as Prefetch Effects
participant GQL as GraphQL<br/>Queries/Mutations
participant Backend as Backend<br/>(NavigationMenuItem)
participant StateStore as State Store<br/>(Recoil)
rect rgb(100, 150, 200)
Note over User,Backend: Initialization Flow
User->>UI: Load app
UI->>FeatureFlag: Check IS_NAVIGATION_MENU_ITEM_ENABLED
FeatureFlag-->>UI: true
UI->>Prefetch: Render PrefetchDataProvider
Prefetch->>GQL: useFindManyNavigationMenuItemsQuery<br/>(if flag enabled)
GQL->>Backend: Query navigationMenuItems
Backend-->>GQL: Return items
GQL->>StateStore: Store in prefetchNavigationMenuItemsState
StateStore-->>Prefetch: Update complete
end
rect rgb(200, 150, 100)
Note over User,Backend: Create Navigation Menu Item
User->>UI: Click "Add to Favorites"
UI->>FeatureFlag: Check flag
FeatureFlag-->>UI: true → use NavigationMenuItem
UI->>Hook: useCreateNavigationMenuItem()
Hook->>StateStore: Get navigationMenuItems
StateStore-->>Hook: Return items
Hook->>Hook: Calculate max position
Hook->>GQL: createNavigationMenuItemMutation
GQL->>Backend: Create with position
Backend-->>GQL: Success
GQL->>GQL: Refetch FindManyNavigationMenuItems
GQL->>Backend: Query updated items
Backend-->>StateStore: Update state
end
rect rgb(150, 200, 100)
Note over User,Backend: Drag & Drop Operation
User->>UI: Drag navigation item
UI->>Hook: useHandleNavigationMenuItemDragAndDrop()
Hook->>Hook: Calculate new position
Hook->>GQL: updateNavigationMenuItemMutation
GQL->>Backend: Update position/folder
Backend-->>GQL: Success
GQL->>GQL: Refetch
StateStore-->>UI: UI updates
end
rect rgb(200, 100, 150)
Note over FeatureFlag,Backend: Fallback Path (Feature Disabled)
FeatureFlag->>Prefetch: is Enabled = false
Prefetch->>GQL: useFindManyRecords (Favorite)
GQL->>Backend: Query Favorite entity
Backend-->>StateStore: Update prefetchFavoritesState
UI->>Hook: useCreateFavorite() fallback
Hook->>GQL: createFavoriteMutation
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Additional Comments (2)
-
packages/twenty-front/src/modules/prefetch/components/PrefetchRunFavoriteQueriesEffect.tsx, line 59-79 (link)style: No error handling for GraphQL query failures. If the favorites query fails, the state won't update and users might see stale/empty data. Consider adding error handling or error state management similar to how other prefetch operations handle failures.
-
packages/twenty-front/src/modules/prefetch/components/PrefetchDataProvider.tsx, line 1-13 (link)logic: Both prefetch effects render unconditionally. The
PrefetchRunFavoriteQueriesEffectincludesskip: isNavigationMenuItemEnabledinternally, but this means Apollo still creates the query instance and watches for changes even when skipped. Consider making the component itself conditional on the feature flag to prevent unnecessary GraphQL operations and hook instantiation.
75 files reviewed, 4 comments
Comment on lines
+35
to
+140
| const handleNavigationMenuItemDragAndDrop: OnDragEndResponder = async ( | ||
| result, | ||
| ) => { | ||
| const { destination, source, draggableId } = result; | ||
|
|
||
| if (!destination) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| destination.droppableId === source.droppableId && | ||
| destination.index === source.index | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const draggedNavigationMenuItem = navigationMenuItems.find( | ||
| (item) => item.id === draggableId, | ||
| ); | ||
| if (!draggedNavigationMenuItem) { | ||
| return; | ||
| } | ||
|
|
||
| const destinationFolderId = validateAndExtractFolderId({ | ||
| droppableId: destination.droppableId, | ||
| orphanDroppableId: ORPHAN_NAVIGATION_MENU_ITEMS_DROPPABLE_ID, | ||
| }); | ||
| const sourceFolderId = validateAndExtractFolderId({ | ||
| droppableId: source.droppableId, | ||
| orphanDroppableId: ORPHAN_NAVIGATION_MENU_ITEMS_DROPPABLE_ID, | ||
| }); | ||
|
|
||
| if ( | ||
| destination.droppableId.startsWith( | ||
| FOLDER_DROPPABLE_IDS.FOLDER_HEADER_PREFIX, | ||
| ) | ||
| ) { | ||
| if (destinationFolderId === null) | ||
| throw new Error('Invalid folder header ID'); | ||
|
|
||
| const folderNavigationMenuItems = navigationMenuItemsSorted.filter( | ||
| (item) => item.folderId === destinationFolderId, | ||
| ); | ||
|
|
||
| const newPosition = | ||
| folderNavigationMenuItems.length === 0 | ||
| ? 1 | ||
| : folderNavigationMenuItems[folderNavigationMenuItems.length - 1] | ||
| .position + 1; | ||
|
|
||
| await updateNavigationMenuItem({ | ||
| id: draggableId, | ||
| folderId: destinationFolderId, | ||
| position: newPosition, | ||
| }); | ||
|
|
||
| openDestinationFolder(destinationFolderId); | ||
| return; | ||
| } | ||
|
|
||
| if (destination.droppableId !== source.droppableId) { | ||
| const destinationNavigationMenuItems = navigationMenuItemsSorted.filter( | ||
| (item) => item.folderId === destinationFolderId, | ||
| ); | ||
|
|
||
| let newPosition; | ||
| if (destinationNavigationMenuItems.length === 0) { | ||
| newPosition = 1; | ||
| } else if (destination.index === 0) { | ||
| newPosition = destinationNavigationMenuItems[0].position - 1; | ||
| } else if (destination.index >= destinationNavigationMenuItems.length) { | ||
| newPosition = | ||
| destinationNavigationMenuItems[ | ||
| destinationNavigationMenuItems.length - 1 | ||
| ].position + 1; | ||
| } else { | ||
| newPosition = calculateNewPosition({ | ||
| destinationIndex: destination.index, | ||
| sourceIndex: -1, | ||
| items: destinationNavigationMenuItems, | ||
| }); | ||
| } | ||
|
|
||
| await updateNavigationMenuItem({ | ||
| id: draggableId, | ||
| folderId: destinationFolderId ?? null, | ||
| position: newPosition, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const navigationMenuItemsInSameList = navigationMenuItemsSorted | ||
| .filter((item) => item.folderId === sourceFolderId) | ||
| .filter((item) => item.id !== draggableId); | ||
|
|
||
| const newPosition = calculateNewPosition({ | ||
| destinationIndex: destination.index, | ||
| sourceIndex: source.index, | ||
| items: navigationMenuItemsInSameList, | ||
| }); | ||
|
|
||
| await updateNavigationMenuItem({ | ||
| id: draggableId, | ||
| position: newPosition, | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
style: The drag-and-drop mutation calls don't include error handling. Consider wrapping updateNavigationMenuItem calls in try-catch to handle failed mutations gracefully, especially since users may lose their drag-drop operation if the mutation fails silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/navigation-menu-item/hooks/useHandleNavigationMenuItemDragAndDrop.ts
Line: 35:140
Comment:
**style:** The drag-and-drop mutation calls don't include error handling. Consider wrapping `updateNavigationMenuItem` calls in try-catch to handle failed mutations gracefully, especially since users may lose their drag-drop operation if the mutation fails silently.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+35
to
+42
| const { data, loading } = useFindManyNavigationMenuItemsQuery({ | ||
| skip: | ||
| showAuthModal || | ||
| isSettingsPage || | ||
| !isWorkspaceActive || | ||
| !isNavigationMenuItemEnabled, | ||
| fetchPolicy: 'cache-and-network', | ||
| }); |
Contributor
There was a problem hiding this comment.
style: Similar to favorites prefetch, there's no error handling for failed NavigationMenuItem queries. If the query fails, the loading state won't properly reflect the error and UI components relying on this data may not behave as expected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/prefetch/components/PrefetchRunNavigationMenuItemQueriesEffect.tsx
Line: 35:42
Comment:
**style:** Similar to favorites prefetch, there's no error handling for failed NavigationMenuItem queries. If the query fails, the loading state won't properly reflect the error and UI components relying on this data may not behave as expected.
How can I resolve this? If you propose a fix, please make it concise.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 75 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/services/twenty-standard-application.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/services/twenty-standard-application.service.ts:91">
P2: Filter existing navigation menu items by folderId === null when computing maxPosition; otherwise items in other folders skew the position for top-level menu items.</violation>
</file>
<file name="packages/twenty-front/src/modules/navigation-menu-item/components/CurrentWorkspaceMemberNavigationMenuItemFolders.tsx">
<violation number="1" location="packages/twenty-front/src/modules/navigation-menu-item/components/CurrentWorkspaceMemberNavigationMenuItemFolders.tsx:66">
P3: Add an accessible label to the icon-only folder button so screen readers can announce the action.</violation>
</file>
<file name="packages/twenty-front/src/modules/navigation-menu-item/hooks/useCreateNavigationMenuItem.ts">
<violation number="1" location="packages/twenty-front/src/modules/navigation-menu-item/hooks/useCreateNavigationMenuItem.ts:37">
P2: Filter the no-folder items by the current workspace member ID; otherwise the position can be calculated using other users’ menu items.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| (item) => | ||
| isDefined(item) && | ||
| item.workspaceId === workspaceId && | ||
| item.userWorkspaceId === null, |
Contributor
There was a problem hiding this comment.
P2: Filter existing navigation menu items by folderId === null when computing maxPosition; otherwise items in other folders skew the position for top-level menu items.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/workspace-manager/twenty-standard-application/services/twenty-standard-application.service.ts, line 91:
<comment>Filter existing navigation menu items by folderId === null when computing maxPosition; otherwise items in other folders skew the position for top-level menu items.</comment>
<file context>
@@ -58,6 +62,81 @@ export class TwentyStandardApplicationService {
+ (item) =>
+ isDefined(item) &&
+ item.workspaceId === workspaceId &&
+ item.userWorkspaceId === null,
+ );
+
</file context>
Suggested change
| item.userWorkspaceId === null, | |
| item.userWorkspaceId === null && item.folderId === null, |
| const relevantItems = folderId | ||
| ? navigationMenuItems.filter((item) => item.folderId === folderId) | ||
| : navigationMenuItems.filter( | ||
| (item) => !item.folderId && item.userWorkspaceId, |
Contributor
There was a problem hiding this comment.
P2: Filter the no-folder items by the current workspace member ID; otherwise the position can be calculated using other users’ menu items.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/navigation-menu-item/hooks/useCreateNavigationMenuItem.ts, line 37:
<comment>Filter the no-folder items by the current workspace member ID; otherwise the position can be calculated using other users’ menu items.</comment>
<file context>
@@ -0,0 +1,59 @@
+ const relevantItems = folderId
+ ? navigationMenuItems.filter((item) => item.folderId === folderId)
+ : navigationMenuItems.filter(
+ (item) => !item.folderId && item.userWorkspaceId,
+ );
+
</file context>
| label={t`Favorites`} | ||
| onClick={toggleNavigationSection} | ||
| rightIcon={ | ||
| <LightIconButton |
Contributor
There was a problem hiding this comment.
P3: Add an accessible label to the icon-only folder button so screen readers can announce the action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/navigation-menu-item/components/CurrentWorkspaceMemberNavigationMenuItemFolders.tsx, line 66:
<comment>Add an accessible label to the icon-only folder button so screen readers can announce the action.</comment>
<file context>
@@ -0,0 +1,84 @@
+ label={t`Favorites`}
+ onClick={toggleNavigationSection}
+ rightIcon={
+ <LightIconButton
+ Icon={IconFolderPlus}
+ onClick={toggleNewFolder}
</file context>
- Updated GraphQL types to include `targetRecord` in `NavigationMenuItem` and related fragments. - Refactored `useSortedNavigationMenuItems` hook to utilize `targetRecord` directly, simplifying the logic. - Implemented `targetRecord` resolver in the server to fetch associated records based on `targetRecordId` and `targetObjectMetadataId`. - Adjusted `NavigationMenuItemDTO` to accommodate the new `targetRecord` field. These changes improve the navigation menu item functionality by allowing direct access to related records.
Contributor
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/engine/metadata-modules/navigation-menu-item/navigation-menu-item.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/navigation-menu-item/navigation-menu-item.service.ts:373">
P1: findTargetRecord bypasses permission checks with a system auth context and no access validation, which can expose records to unauthorized callers. Prefer enforcing permission checks or requiring a caller auth context.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| workspaceId, | ||
| objectMetadata.nameSingular, | ||
| { | ||
| shouldBypassPermissionChecks: true, |
Contributor
There was a problem hiding this comment.
P1: findTargetRecord bypasses permission checks with a system auth context and no access validation, which can expose records to unauthorized callers. Prefer enforcing permission checks or requiring a caller auth context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/metadata-modules/navigation-menu-item/navigation-menu-item.service.ts, line 373:
<comment>findTargetRecord bypasses permission checks with a system auth context and no access validation, which can expose records to unauthorized callers. Prefer enforcing permission checks or requiring a caller auth context.</comment>
<file context>
@@ -334,4 +337,49 @@ export class NavigationMenuItemService {
+ workspaceId,
+ objectMetadata.nameSingular,
+ {
+ shouldBypassPermissionChecks: true,
+ },
+ );
</file context>
Contributor
There was a problem hiding this comment.
Indeed, we should not bypass permission checks here
Contributor
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Contributor
There was a problem hiding this comment.
Bravo ! 👏
Left few comments. I tested and it globally works.
I haven't dived in all FE files, I'll do it tomorrow but if I understand it's very close to (old) favorite files.
Bugs seen :
- when adding a favorite in a folder - then reloading page - then clicking on folder, it does not open (as it is empty)
- when adding a view in favorite - nothing happens (I think it's not migrated yet)
Comments :
- Folders can't be re-ordered (but same in favorites - out of this PR scope)
Concerning CI :
- You need to rebase on main and run gql codegen
...wenty-front/src/modules/navigation-menu-item/graphql/fragments/navigationMenuItemFragment.ts
Outdated
Show resolved
Hide resolved
| return ( | ||
| <> | ||
| <PrefetchRunFavoriteQueriesEffect /> | ||
| <PrefetchRunNavigationMenuItemQueriesEffect /> |
Contributor
There was a problem hiding this comment.
Now favorite/navigationMenuItem are in core schema, why not fetch them through currentUser ? in currentWorkspace ?
Contributor
Author
There was a problem hiding this comment.
I tried fetching navigation menu items through currentUser.currentWorkspace.navigationMenuItems, but PrefetchRunNavigationMenuItemQueriesEffect with FindManyNavigationMenuItems works better because refetching GetCurrentUser after mutations is slower since it fetches entire user/workspace data vs a targeted query
| updatedAt: Date; | ||
|
|
||
| @Field(() => GraphQLJSON, { nullable: true }) | ||
| targetRecord?: Record<string, unknown> | null; |
Contributor
There was a problem hiding this comment.
targetRecordIdentifier should be better ? with label and imageUrl as in SearchRecordDTO ? to take advantage of gql schema
| workspaceId, | ||
| objectMetadata.nameSingular, | ||
| { | ||
| shouldBypassPermissionChecks: true, |
Contributor
There was a problem hiding this comment.
Indeed, we should not bypass permission checks here
| } | ||
|
|
||
| @ResolveField(() => GraphQLJSON, { nullable: true }) | ||
| async targetRecord( |
Contributor
There was a problem hiding this comment.
targetRecordIdentifier is enough. You can inspire from search resolver with searchRecord format
Contributor
|
@abdulrahmancodes I don't understand the seeded navigationMenuItems in core.navigationMenuItem table after resetting my database |
- Added `FilesField` and `TemporaryFilesField` to the FileFolder enum in both generated GraphQL files.
- Removed `targetRecord` from `NAVIGATION_MENU_ITEM_FRAGMENT`. - Introduced `NAVIGATION_MENU_ITEM_QUERY_FRAGMENT` to include `targetRecord`. - Updated `findManyNavigationMenuItems` and `findOneNavigationMenuItem` queries to use the new query fragment.
| if (droppableId === FAVORITE_DROPPABLE_IDS.ORPHAN_FAVORITES) { | ||
| type ValidateAndExtractFolderIdParams = { | ||
| droppableId: string; | ||
| orphanDroppableId: string; |
Contributor
There was a problem hiding this comment.
nit : TODO : not forget to remove this extra orphanDroppableId prop when deleting all favorites code
| (navigationMenuItem.Icon ? getIcon(navigationMenuItem.Icon) : undefined); | ||
| const iconColorToUse = StandardIcon ? IconColor : theme.font.color.secondary; | ||
|
|
||
| const placeholderColorSeed = navigationMenuItem.targetRecordId ?? undefined; |
Contributor
There was a problem hiding this comment.
nit : navigationMenuItem.targetRecordId is already string or undefined, no ?
| const targetRecordsMap = new Map<string, ObjectRecord>(); | ||
| itemsInFolder.forEach((item) => { | ||
| const itemTargetRecordId = item.targetRecordId; | ||
| if (!isDefined(itemTargetRecordId) || isDefined(item.viewId)) { |
Contributor
There was a problem hiding this comment.
Redundant no ? if itemTargetRecordId not defined for an item in folder then viewId should be defined
|
|
||
| const { navigationMenuItems } = usePrefetchedNavigationMenuItemsData(); | ||
|
|
||
| const navigationMenuItemsByFolder = useMemo(() => { |
Contributor
There was a problem hiding this comment.
Good idea to have a useMemo here but not sure its works. It seems to still trigger too many times
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implement Navigation Menu Items Frontend
Implements the frontend for navigation menu items, the new system replacing favorites.
Changes
Migration Note
The favorites and navigation menu item modules currently exist in parallel. The favorites code will be removed once all data has been migrated to navigation menu items.