-
Notifications
You must be signed in to change notification settings - Fork 3
Config refactor #369
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
base: main
Are you sure you want to change the base?
Config refactor #369
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,3 @@ | ||
| { | ||
| "mcpServers": { | ||
| "nx-mcp": { | ||
| "url": "http://localhost:9216/sse" | ||
| } | ||
| } | ||
| "mcpServers": {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,23 +8,14 @@ export const getPluginsRoutes = () => { | |
| const [pluginsMetaData] = useAtom(pluginsConfigState); | ||
| const plugins = Object.values(pluginsMetaData || {}); | ||
|
|
||
| const allModules = plugins.flatMap((plugin) => | ||
| plugin.modules | ||
| .filter((module) => !module.settingsOnly) | ||
| .map((module) => ({ | ||
| ...module, | ||
| pluginName: plugin.name, | ||
| })), | ||
| ); | ||
|
|
||
| return allModules.map((module) => ( | ||
| return plugins.map((module) => ( | ||
| <Route | ||
| key={module.name} | ||
| path={`/${module.path}/*`} | ||
| element={ | ||
| <RenderPluginsComponent | ||
| moduleName={module.name} | ||
| pluginName={`${module.pluginName}_ui`} | ||
| pluginName={`${module.name}_ui`} | ||
| remoteModuleName={module.name} | ||
| /> | ||
| } | ||
|
|
@@ -36,24 +27,17 @@ export const getPluginsSettingsRoutes = () => { | |
| const [pluginsMetaData] = useAtom(pluginsConfigState); | ||
| const plugins = Object.values(pluginsMetaData || {}); | ||
|
|
||
| const settingsModules = plugins.flatMap((plugin) => | ||
| plugin.modules | ||
| .filter((module) => module.hasSettings || module.settingsOnly) | ||
| .map((module) => ({ | ||
| ...module, | ||
| pluginName: plugin.name, | ||
| })), | ||
| ); | ||
| console.log(plugins); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. syntax: Remove debug console.log statement before merging Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/core-ui/src/modules/app/hooks/usePluginsRouter.tsx
Line: 30:30
Comment:
**syntax:** Remove debug console.log statement before merging
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
|
Comment on lines
+30
to
31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Remove the stray Per our frontend guidelines we avoid console logging in shipped code. Please drop this log (or replace with structured telemetry if needed). 🤖 Prompt for AI Agents |
||
| return settingsModules.map((module) => ( | ||
| return plugins.map((plugin) => ( | ||
| <Route | ||
| key={module.name} | ||
| path={`/${module.path}/*`} | ||
| key={plugin.name} | ||
| path={`/${plugin.path}/*`} | ||
| element={ | ||
| <RenderPluginsComponent | ||
| moduleName={module.name} | ||
| pluginName={`${module.pluginName}_ui`} | ||
| remoteModuleName={`${module.name}Settings`} | ||
| moduleName={plugin.name} | ||
| pluginName={`${plugin.name}_ui`} | ||
| remoteModuleName={`${plugin.name}Settings`} | ||
| /> | ||
| } | ||
| /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,6 +1,6 @@ | ||||||
| import { IconChevronLeft } from '@tabler/icons-react'; | ||||||
| import { activePluginState, NavigationMenuGroup, Sidebar } from 'erxes-ui'; | ||||||
| import { usePluginsNavigationGroups } from '../hooks/usePluginsModules'; | ||||||
| import { usePluginsNavigationGroups } from '../hooks/usePlugins'; | ||||||
| import { useAtom } from 'jotai'; | ||||||
|
|
||||||
| export const NavigationPluginExitButton = () => { | ||||||
|
|
@@ -36,6 +36,7 @@ export const NavigationPlugins = () => { | |||||
| } | ||||||
|
|
||||||
| if (activePlugin && navigationGroups[activePlugin]) { | ||||||
| const subGroups = navigationGroups[activePlugin].subGroups; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (code-quality): Prefer object destructuring when accessing and using properties. (
Suggested change
ExplanationObject destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide |
||||||
| return ( | ||||||
| <> | ||||||
| <NavigationMenuGroup | ||||||
|
|
@@ -50,7 +51,7 @@ export const NavigationPlugins = () => { | |||||
| <Content key={index} /> | ||||||
| ))} | ||||||
| </NavigationMenuGroup> | ||||||
| {navigationGroups[activePlugin].subGroups.map((SubGroup, index) => ( | ||||||
| {subGroups.map((SubGroup, index) => ( | ||||||
| <SubGroup key={index} /> | ||||||
| ))} | ||||||
| </> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,5 @@ | ||
| import { useQuery } from '@apollo/client'; | ||
| import { GET_FAVORITES } from '@/navigation/graphql/queries/getFavorites'; | ||
| import { usePluginsModules } from '@/navigation/hooks/usePluginsModules'; | ||
| import { useAtomValue } from 'jotai'; | ||
| import { currentUserState } from 'ui-modules'; | ||
|
|
||
|
|
@@ -21,7 +20,6 @@ interface GetFavoritesResponse { | |
| } | ||
|
|
||
| export function useFavorites(): FavoriteModule[] { | ||
| const modules = usePluginsModules(); | ||
| const currentUser = useAtomValue(currentUserState); | ||
|
|
||
| const { data } = useQuery<GetFavoritesResponse>(GET_FAVORITES, { | ||
|
|
@@ -30,39 +28,5 @@ export function useFavorites(): FavoriteModule[] { | |
|
|
||
| const favorites = data?.getFavoritesByCurrentUser ?? []; | ||
|
Comment on lines
28
to
31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): useFavorites now always returns an empty array. This change will hide all favorite modules. If this is intentional or temporary, please clarify with a TODO or plan to restore the original logic. |
||
|
|
||
| return favorites.reduce<FavoriteModule[]>((acc, favorite) => { | ||
| if (favorite.type === 'module') { | ||
| const module = modules.find( | ||
| (m) => m.path === favorite.path.replace('/', ''), | ||
| ); | ||
|
|
||
| if (module) { | ||
| acc.push({ | ||
| name: module.name, | ||
| icon: module.icon, | ||
| path: module.path, | ||
| }); | ||
| } else { | ||
| const moduleWithSubmenu = modules.find( | ||
| (m) => m.path === favorite.path.split('/')[1], | ||
| ); | ||
|
|
||
| if (moduleWithSubmenu?.submenus) { | ||
| const matchingSubmenu = moduleWithSubmenu.submenus.find( | ||
| (sub) => sub.path === favorite.path.replace('/', ''), | ||
| ); | ||
|
|
||
| if (matchingSubmenu) { | ||
| acc.push({ | ||
| name: matchingSubmenu.name, | ||
| icon: matchingSubmenu.icon || moduleWithSubmenu.icon, | ||
| path: matchingSubmenu.path, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return acc; | ||
| }, []); | ||
| return []; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Favorites functionality has been completely removed - the hook now returns an empty array instead of processing favorites data. This will break any UI components that rely on displaying user favorites. Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/core-ui/src/modules/navigation/hooks/useFavorites.tsx
Line: 31:31
Comment:
**logic:** Favorites functionality has been completely removed - the hook now returns an empty array instead of processing favorites data. This will break any UI components that rely on displaying user favorites.
How can I resolve this? If you propose a fix, please make it concise. |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -61,7 +61,7 @@ export const usePluginsNavigationGroups = (): NavigationGroups => { | |||||||||||||||||||||||||||
| ? [...existingGroup.contents, newContent] | ||||||||||||||||||||||||||||
| : existingGroup.contents; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const newSubGroup = plugin.navigationGroup?.subGroups; | ||||||||||||||||||||||||||||
| const newSubGroup = plugin.navigationGroup?.subGroup; | ||||||||||||||||||||||||||||
| const updatedSubGroups = newSubGroup | ||||||||||||||||||||||||||||
| ? [...existingGroup.subGroups, newSubGroup] | ||||||||||||||||||||||||||||
| : existingGroup.subGroups; | ||||||||||||||||||||||||||||
|
Comment on lines
-64
to
67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: subGroups replaced with subGroup in navigationGroup. Ensure subGroup is consistently treated as an array to avoid issues when handling multiple subgroups.
Suggested change
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -80,22 +80,9 @@ export function SettingsSidebar() { | |||||||||||||||||||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||||||||||||||||||||
| </SettingsNavigationGroup> | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| {Array.from(pluginsWithSettingsModules.entries()).map( | ||||||||||||||||||||||||||||||||||||||||||
| ([pluginName, modules]) => ( | ||||||||||||||||||||||||||||||||||||||||||
| <SettingsNavigationGroup | ||||||||||||||||||||||||||||||||||||||||||
| key={pluginName} | ||||||||||||||||||||||||||||||||||||||||||
| name={pluginName.charAt(0).toUpperCase() + pluginName.slice(1)} | ||||||||||||||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||||||||||||||
| {modules.map((item) => ( | ||||||||||||||||||||||||||||||||||||||||||
| <NavigationMenuLinkItem | ||||||||||||||||||||||||||||||||||||||||||
| key={item.name} | ||||||||||||||||||||||||||||||||||||||||||
| pathPrefix={AppPath.Settings} | ||||||||||||||||||||||||||||||||||||||||||
| path={item.path} | ||||||||||||||||||||||||||||||||||||||||||
| name={item.name} | ||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||||||||||||||||||||
| </SettingsNavigationGroup> | ||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||
| {pluginSettingsNavigations.map( | ||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable 'pluginSettingsNavigations' is used but not defined or imported. Verify its source to avoid runtime errors. |
||||||||||||||||||||||||||||||||||||||||||
| (SettingsNavigation, index) => | ||||||||||||||||||||||||||||||||||||||||||
| SettingsNavigation && <SettingsNavigation key={index} />, | ||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+83
to
86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/core-ui/src/modules/settings/components/SettingsSidebar.tsx
Line: 83:86
Comment:
**logic:** `pluginSettingsNavigations` is not defined. You need to either import it or create it from plugin configs with `settingsNavigation` functions.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+83
to
86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Define
- return (
+ const pluginSettingsNavigations = useMemo(
+ () =>
+ Object.values(pluginsMetaData || {})
+ .map((plugin) => plugin.settingsNavigation)
+ .filter(
+ (navigation): navigation is NonNullable<typeof navigation> =>
+ Boolean(navigation),
+ ),
+ [pluginsMetaData],
+ );
+
+ return (
@@
- {pluginSettingsNavigations.map(
- (SettingsNavigation, index) =>
- SettingsNavigation && <SettingsNavigation key={index} />,
- )}
+ {pluginSettingsNavigations.map((SettingsNavigation, index) => (
+ <SettingsNavigation key={index} />
+ ))}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
| </Sidebar.Content> | ||||||||||||||||||||||||||||||||||||||||||
| </> | ||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -109,7 +96,6 @@ export const SettingsNavigationGroup = ({ | |||||||||||||||||||||||||||||||||||||||||
| name: string; | ||||||||||||||||||||||||||||||||||||||||||
| children: React.ReactNode; | ||||||||||||||||||||||||||||||||||||||||||
| }) => { | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| if (React.Children.count(children) === 0) return null; | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,12 +10,5 @@ export const useFloatingWidgetsModules = () => { | |
|
|
||
| const plugins = Object.values(pluginsMetaData); | ||
|
|
||
| return plugins.flatMap((plugin) => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue (bug_risk): Floating widgets now filter plugins instead of modules. This change may prevent floating widgets defined in modules from being included, potentially breaking existing functionality if widgets are meant to be module-specific. |
||
| plugin.modules | ||
| .filter((module) => module.hasFloatingWidget) | ||
| .map((module) => ({ | ||
| ...module, | ||
| pluginName: plugin.name, | ||
| })), | ||
| ); | ||
| return plugins.filter((plugin) => plugin.hasFloatingWidget); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: This structural change from module-level to plugin-level filtering will break if any consuming components still expect the old data structure with Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/core-ui/src/modules/widgets/hooks/useFloatingWidgetsModules.tsx
Line: 13:13
Comment:
**logic:** This structural change from module-level to plugin-level filtering will break if any consuming components still expect the old data structure with `pluginName` metadata and module-level objects
How can I resolve this? If you propose a fix, please make it concise. |
||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -22,6 +22,8 @@ export function RenderPluginsComponent({ | |||
| const [isLoading, setIsLoading] = useState(true); | ||||
| const [hasError, setHasError] = useState<{ message: string } | null>(null); | ||||
|
|
||||
| console.log(pluginName, remoteModuleName, moduleName); | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Console log should be removed per project guidelines that explicitly state 'Avoid console logs' Context Used: Context from Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/core-ui/src/plugins/components/RenderPluginsComponent.tsx
Line: 25:25
Comment:
**style:** Console log should be removed per project guidelines that explicitly state 'Avoid console logs'
**Context Used:** Context from `dashboard` - .cursorrules ([source](https://app.greptile.com/review/custom-context?memory=bf691a04-8aaa-40fe-8db5-b17d5fe3755f))
How can I resolve this? If you propose a fix, please make it concise. |
||||
|
|
||||
|
Comment on lines
+25
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the console.log statement. The coding guidelines specify to avoid console logs in production code. This debug statement should be removed. As per coding guidelines Apply this diff to remove the debug statement: - console.log(pluginName, remoteModuleName, moduleName);
-📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||
| useEffect(() => { | ||||
| const loadPlugin = async () => { | ||||
| try { | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,6 @@ import { | |
| IconSpiral, | ||
| IconUser, | ||
| } from '@tabler/icons-react'; | ||
| import { IUIConfig } from 'erxes-ui'; | ||
|
|
||
| export const GET_CORE_MODULES = (version?: boolean): IUIConfig['modules'] => { | ||
| const MODULES: IUIConfig['modules'] = [ | ||
|
Comment on lines
14
to
15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. syntax: TypeScript error: IUIConfig is not defined. The import was removed but the type is still referenced in the return type and variable declaration. Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/core-ui/src/plugins/constants/core-plugins.constants.ts
Line: 14:15
Comment:
**syntax:** TypeScript error: IUIConfig is not defined. The import was removed but the type is still referenced in the return type and variable declaration.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,7 +27,10 @@ export const NavigationMenuLinkItem = forwardRef< | |||||||||||
| ) => { | ||||||||||||
| const { pathname } = useLocation(); | ||||||||||||
| const fullPath = pathPrefix ? `${pathPrefix}/${path}` : path; | ||||||||||||
| const isActive = pathname.startsWith(`/${fullPath}`); | ||||||||||||
|
|
||||||||||||
| const isActive = pathname.startsWith( | ||||||||||||
| fullPath?.startsWith('/') ? fullPath : `/${fullPath}`, | ||||||||||||
| ); | ||||||||||||
| return ( | ||||||||||||
| <Sidebar.MenuItem> | ||||||||||||
| <Sidebar.MenuButton | ||||||||||||
|
|
@@ -57,6 +60,21 @@ export const NavigationMenuLinkItem = forwardRef< | |||||||||||
|
|
||||||||||||
| NavigationMenuLinkItem.displayName = 'NavigationMenuLinkItem'; | ||||||||||||
|
|
||||||||||||
| export const SettingsNavigationMenuLinkItem = forwardRef< | ||||||||||||
| React.ElementRef<typeof Sidebar.MenuButton>, | ||||||||||||
| React.ComponentProps<typeof NavigationMenuLinkItem> | ||||||||||||
| >(({ pathPrefix, ...props }, ref) => { | ||||||||||||
| const settingsPathPrefix = | ||||||||||||
| '/settings' + (pathPrefix?.startsWith('/') ? pathPrefix : `/${pathPrefix}`); | ||||||||||||
|
Comment on lines
+67
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: The path handling logic duplicates the same pattern used in NavigationMenuLinkItem. Consider extracting this into a utility function to avoid repetition. Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/libs/erxes-ui/src/modules/navigation-menu/components/NavigationMenu.tsx
Line: 67:68
Comment:
**style:** The path handling logic duplicates the same pattern used in NavigationMenuLinkItem. Consider extracting this into a utility function to avoid repetition.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+67
to
+68
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle undefined pathPrefix safely. If Apply this diff to fix the issue: - const settingsPathPrefix =
- '/settings' + (pathPrefix?.startsWith('/') ? pathPrefix : `/${pathPrefix}`);
+ const settingsPathPrefix = pathPrefix
+ ? '/settings' + (pathPrefix.startsWith('/') ? pathPrefix : `/${pathPrefix}`)
+ : '/settings';📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| return ( | ||||||||||||
| <NavigationMenuLinkItem | ||||||||||||
| {...props} | ||||||||||||
| pathPrefix={settingsPathPrefix} | ||||||||||||
| ref={ref} | ||||||||||||
| /> | ||||||||||||
| ); | ||||||||||||
| }); | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Missing displayName assignment for SettingsNavigationMenuLinkItem component, which is inconsistent with the pattern used for other components in this file.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: frontend/libs/erxes-ui/src/modules/navigation-menu/components/NavigationMenu.tsx
Line: 76:76
Comment:
**style:** Missing displayName assignment for SettingsNavigationMenuLinkItem component, which is inconsistent with the pattern used for other components in this file.
```suggestion
});
SettingsNavigationMenuLinkItem.displayName = 'SettingsNavigationMenuLinkItem';
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||
|
|
||||||||||||
| export const NavigationMenuItem = forwardRef< | ||||||||||||
| React.ElementRef<typeof Sidebar.MenuButton>, | ||||||||||||
| React.ComponentProps<typeof Sidebar.MenuButton> & { | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Variable name 'module' is misleading since this now maps over plugins directly. Consider renaming to 'plugin' for consistency with getPluginsSettingsRoutes
Prompt To Fix With AI