Skip to content
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.

Sign up for GitHub

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

chore: 后端路由模式时，本地静态路由和后端路由合并 #4899

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore: 后端路由模式时，本地静态路由和后端路由合并 #4899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion apps/web-antd/src/router/access.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ import type {

import { generateAccessible } from '@vben/access';
import { preferences } from '@vben/preferences';
import { cloneDeep } from '@vben/utils';

import { message } from 'ant-design-vue';

import { getAllMenusApi } from '#/api';
import { BasicLayout, IFrameView } from '#/layouts';
import { $t } from '#/locales';

import { staticMenuList } from './routes/static';

const forbiddenComponent = () => import('#/views/_core/fallback/forbidden.vue');

async function generateAccess(options: GenerateMenuAndRoutesOptions) {
Expand All @@ -29,7 +32,10 @@ async function generateAccess(options: GenerateMenuAndRoutesOptions) {
content: `${$t('common.loadingMenu')}...`,
duration: 1.5,
});
return await getAllMenusApi();

const dynamicMenus = await getAllMenusApi();
// 本地菜单和动态菜单合并
return [...cloneDeep(staticMenuList), ...dynamicMenus];
Comment on lines +36 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding validation and deduplication logic

The current implementation merges static and dynamic menus without validation. This could lead to:

  1. Duplicate menu items if the same routes exist in both lists
  2. Potential conflicts in menu hierarchy
  3. Performance impact from deep cloning large menu structures

Consider implementing the following improvements:

- return [...cloneDeep(staticMenuList), ...dynamicMenus];
+ const staticMenus = cloneDeep(staticMenuList);
+ // Validate menu structure
+ const validatedDynamicMenus = validateMenuStructure(dynamicMenus);
+ // Remove duplicates based on route path or menu key
+ const uniqueMenus = deduplicateMenus(staticMenus, validatedDynamicMenus);
+ return uniqueMenus;

Consider translating the Chinese comment to English for consistency:

- // 本地菜单和动态菜单合并
+ // Merge local static menus with dynamic menus
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const dynamicMenus = await getAllMenusApi();
// 本地菜单和动态菜单合并
return [...cloneDeep(staticMenuList), ...dynamicMenus];
const dynamicMenus = await getAllMenusApi();
// Merge local static menus with dynamic menus
return [...cloneDeep(staticMenuList), ...dynamicMenus];

},
// 可以指定没有权限跳转403页面
forbiddenComponent,
Expand Down
73 changes: 73 additions & 0 deletions apps/web-antd/src/router/routes/static.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import type { RouteRecordStringComponent } from '@vben/types';

import { $t } from '@vben/locales';

/**
* 该文件放非后台返回的前端静态路由
*/

/**
* demo
*/
const demoRoute: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
hideChildrenInMenu: true,
icon: 'lucide:copyright',
order: 9999,
title: $t('demos.vben.about'),
},
name: 'About',
path: '/about',
children: [
{
component: '/_core/about/index',
meta: {
title: $t('demos.vben.about'),
},
name: 'VbenAbout',
path: '/vben-admin/about',
Comment on lines +22 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Review path inconsistency

The parent route path is /about but the child route path is /vben-admin/about. This inconsistency might cause routing issues.

   path: '/about',
   children: [
     {
       component: '/_core/about/index',
       meta: {
         title: $t('demos.vben.about'),
       },
       name: 'VbenAbout',
-      path: '/vben-admin/about',
+      path: 'index',
     },
   ],
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
path: '/about',
children: [
{
component: '/_core/about/index',
meta: {
title: $t('demos.vben.about'),
},
name: 'VbenAbout',
path: '/vben-admin/about',
path: '/about',
children: [
{
component: '/_core/about/index',
meta: {
title: $t('demos.vben.about'),
},
name: 'VbenAbout',
path: 'index',

},
],
},
];

/**
* 这里放本地路由
*/
export const staticMenuList: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
icon: 'lucide:layout-dashboard',
order: -1,
title: 'page.dashboard.title',
},
name: 'Dashboard',
path: '/',
redirect: '/analytics',
children: [
{
name: 'Analytics',
path: '/analytics',
component: '/dashboard/analytics/index',
meta: {
affixTab: true,
icon: 'lucide:area-chart',
title: 'page.dashboard.analytics',
},
},
{
name: 'Workspace',
path: '/workspace',
component: '/dashboard/workspace/index',
meta: {
icon: 'carbon:workspace',
title: 'page.dashboard.workspace',
},
},
],
},
...demoRoute,
];
8 changes: 7 additions & 1 deletion apps/web-ele/src/router/access.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,16 @@ import type {

import { generateAccessible } from '@vben/access';
import { preferences } from '@vben/preferences';
import { cloneDeep } from '@vben/utils';

import { ElMessage } from 'element-plus';

import { getAllMenusApi } from '#/api';
import { BasicLayout, IFrameView } from '#/layouts';
import { $t } from '#/locales';

import { staticMenuList } from './routes/static';

const forbiddenComponent = () => import('#/views/_core/fallback/forbidden.vue');

async function generateAccess(options: GenerateMenuAndRoutesOptions) {
Expand All @@ -29,7 +32,10 @@ async function generateAccess(options: GenerateMenuAndRoutesOptions) {
duration: 1500,
message: `${$t('common.loadingMenu')}...`,
});
return await getAllMenusApi();

const dynamicMenus = await getAllMenusApi();
// 本地菜单和动态菜单合并
return [...cloneDeep(staticMenuList), ...dynamicMenus];
Comment on lines +36 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for getAllMenusApi.

The API call could fail, but there's no error handling. Consider adding try-catch to handle potential failures gracefully.

-      const dynamicMenus = await getAllMenusApi();
-      // 本地菜单和动态菜单合并
-      return [...cloneDeep(staticMenuList), ...dynamicMenus];
+      try {
+        const dynamicMenus = await getAllMenusApi();
+        // Merge local static menus with dynamic menus
+        return [...cloneDeep(staticMenuList), ...dynamicMenus];
+      } catch (error) {
+        ElMessage.error($t('common.menuLoadError'));
+        console.error('Failed to load dynamic menus:', error);
+        // Fallback to static menus only
+        return cloneDeep(staticMenuList);
+      }

Committable suggestion skipped: line range outside the PR's diff.

},
// 可以指定没有权限跳转403页面
forbiddenComponent,
Expand Down
73 changes: 73 additions & 0 deletions apps/web-ele/src/router/routes/static.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import type { RouteRecordStringComponent } from '@vben/types';

import { $t } from '@vben/locales';

/**
* 该文件放非后台返回的前端静态路由
*/

/**
* demo
*/
const demoRoute: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
hideChildrenInMenu: true,
icon: 'lucide:copyright',
order: 9999,
title: $t('demos.vben.about'),
},
name: 'About',
path: '/about',
children: [
{
component: '/_core/about/index',
meta: {
title: $t('demos.vben.about'),
},
name: 'VbenAbout',
path: '/vben-admin/about',
},
],
},
];
Comment on lines +12 to +34
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix inconsistent route path structure

The current path structure might cause routing issues:

  • Parent path is /about
  • Child path is /vben-admin/about
  • Child path should extend the parent path pattern

Also, hideChildrenInMenu: true might be unnecessary since there's only one child route.

Consider this fix:

 const demoRoute: RouteRecordStringComponent[] = [
   {
     component: 'BasicLayout',
     meta: {
-      hideChildrenInMenu: true,
       icon: 'lucide:copyright',
       order: 9999,
       title: $t('demos.vben.about'),
     },
     name: 'About',
-    path: '/about',
+    path: '/vben-admin',
     children: [
       {
         component: '/_core/about/index',
         meta: {
           title: $t('demos.vben.about'),
         },
         name: 'VbenAbout',
         path: '/vben-admin/about',
       },
     ],
   },
 ];
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const demoRoute: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
hideChildrenInMenu: true,
icon: 'lucide:copyright',
order: 9999,
title: $t('demos.vben.about'),
},
name: 'About',
path: '/about',
children: [
{
component: '/_core/about/index',
meta: {
title: $t('demos.vben.about'),
},
name: 'VbenAbout',
path: '/vben-admin/about',
},
],
},
];
const demoRoute: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
icon: 'lucide:copyright',
order: 9999,
title: $t('demos.vben.about'),
},
name: 'About',
path: '/vben-admin',
children: [
{
component: '/_core/about/index',
meta: {
title: $t('demos.vben.about'),
},
name: 'VbenAbout',
path: '/vben-admin/about',
},
],
},
];


/**
* 这里放本地路由
*/
export const staticMenuList: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
icon: 'lucide:layout-dashboard',
order: -1,
title: 'page.dashboard.title',
},
name: 'Dashboard',
path: '/',
redirect: '/analytics',
children: [
{
name: 'Analytics',
path: '/analytics',
component: '/dashboard/analytics/index',
meta: {
affixTab: true,
icon: 'lucide:area-chart',
title: 'page.dashboard.analytics',
},
},
{
name: 'Workspace',
path: '/workspace',
component: '/dashboard/workspace/index',
meta: {
icon: 'carbon:workspace',
title: 'page.dashboard.workspace',
},
},
],
},
...demoRoute,
];
8 changes: 7 additions & 1 deletion apps/web-naive/src/router/access.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@ import type {

import { generateAccessible } from '@vben/access';
import { preferences } from '@vben/preferences';
import { cloneDeep } from '@vben/utils';

import { message } from '#/adapter/naive';
import { getAllMenusApi } from '#/api';
import { BasicLayout, IFrameView } from '#/layouts';
import { $t } from '#/locales';

import { staticMenuList } from './routes/static';

const forbiddenComponent = () => import('#/views/_core/fallback/forbidden.vue');

async function generateAccess(options: GenerateMenuAndRoutesOptions) {
Expand All @@ -27,7 +30,10 @@ async function generateAccess(options: GenerateMenuAndRoutesOptions) {
message.loading(`${$t('common.loadingMenu')}...`, {
duration: 1.5,
});
return await getAllMenusApi();

const dynamicMenus = await getAllMenusApi();
// 本地菜单和动态菜单合并
return [...cloneDeep(staticMenuList), ...dynamicMenus];
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and translate comment

  1. Consider adding error handling for getAllMenusApi() to gracefully handle API failures.
  2. The Chinese comment should be translated to English for consistency.

Apply this diff:

-      const dynamicMenus = await getAllMenusApi();
-      // 本地菜单和动态菜单合并
-      return [...cloneDeep(staticMenuList), ...dynamicMenus];
+      try {
+        const dynamicMenus = await getAllMenusApi();
+        // Merge local static menus with dynamic menus
+        return [...cloneDeep(staticMenuList), ...dynamicMenus];
+      } catch (error) {
+        message.error(`${$t('common.loadingMenuFailed')}`);
+        console.error('Failed to fetch dynamic menus:', error);
+        // Fallback to static menus only
+        return cloneDeep(staticMenuList);
+      }

Committable suggestion skipped: line range outside the PR's diff.

},
// 可以指定没有权限跳转403页面
forbiddenComponent,
Expand Down
73 changes: 73 additions & 0 deletions apps/web-naive/src/router/routes/static.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import type { RouteRecordStringComponent } from '@vben/types';

import { $t } from '@vben/locales';

/**
* 该文件放非后台返回的前端静态路由
*/

/**
* demo
*/
const demoRoute: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
hideChildrenInMenu: true,
icon: 'lucide:copyright',
order: 9999,
title: $t('demos.vben.about'),
},
name: 'About',
path: '/about',
children: [
{
component: '/_core/about/index',
meta: {
title: $t('demos.vben.about'),
},
name: 'VbenAbout',
path: '/vben-admin/about',
},
],
},
];

/**
* 这里放本地路由
*/
export const staticMenuList: RouteRecordStringComponent[] = [
{
component: 'BasicLayout',
meta: {
icon: 'lucide:layout-dashboard',
order: -1,
title: 'page.dashboard.title',
},
name: 'Dashboard',
path: '/',
redirect: '/analytics',
children: [
{
name: 'Analytics',
path: '/analytics',
component: '/dashboard/analytics/index',
meta: {
affixTab: true,
icon: 'lucide:area-chart',
title: 'page.dashboard.analytics',
},
},
{
name: 'Workspace',
path: '/workspace',
component: '/dashboard/workspace/index',
meta: {
icon: 'carbon:workspace',
title: 'page.dashboard.workspace',
},
},
],
},
...demoRoute,
];