Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions ui/src/permission/modules/homepage/homepage.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/** 工作空间「概览」按钮权限。* */

import { can } from '../../policy'
import { PermissionConstants as P } from '../../core'

export default {
/**
* 工作空间「概览」只读权限
*/
read: () => can(P.HOMEPAGE_READ),
/**
* 工作空间「概览」导出权限
*/
edit: () => can(P.HOMEPAGE_EXPORT)
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The code is syntactically fine, but there are a few semantic/style issues:

  1. Likely permission mismatch

    • edit is mapped to P.HOMEPAGE_EXPORT:
      edit: () => can(P.HOMEPAGE_EXPORT)
    • The JSDoc says this is the export permission, not edit permission.
    • This is probably a copy/paste bug. If consumers check permissions.edit, they will incorrectly get export permission. If the intent is export, rename the key to export (or exportPermission) instead of edit.
  2. Naming inconsistency

    • The object exposes read and edit, but the second permission is actually export.
    • Prefer:
      read: () => can(P.HOMEPAGE_READ),
      export: () => can(P.HOMEPAGE_EXPORT)
    • If edit is part of an existing public API, keep it only if it really maps to an edit permission, or add an explicit alias with a comment.
  3. Malformed/odd JSDoc header

    • This comment:
      /** 工作空间「概览」按钮权限。* */
      has an extra * before */. It is harmless, but cleaner is:
      /** 工作空间「概览」按钮权限。 */
  4. Add return types if using TypeScript

    • More explicit and safer:
      read: (): boolean => can(P.HOMEPAGE_READ)
  5. Potential circular dependency / barrel import risk

    • Imports from ../../policy and ../../core may be fine, but if those are barrel files that re-export this module, watch for circular dependencies. Direct imports can be safer in permission modules.

Suggested corrected version:

/** 工作空间「概览」按钮权限。 */

import { can } from '../../policy'
import { PermissionConstants as P } from '../../core'

export default {
  /** 工作空间「概览」只读权限 */
  read: (): boolean => can(P.HOMEPAGE_READ),

  /** 工作空间「概览」导出权限 */
  export: (): boolean => can(P.HOMEPAGE_EXPORT)
}

If edit must remain for compatibility, then verify whether it should be:

edit: (): boolean => can(P.HOMEPAGE_EDIT)

rather than P.HOMEPAGE_EXPORT.

10 changes: 10 additions & 0 deletions ui/src/permission/modules/homepage/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/** 「概览」在各场景下的按钮权限。* */

import homepage from './homepage'

export default {
/**
* 工作空间「概览」权限
*/
homepage
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The code is syntactically valid JavaScript, but there are a few points to check:

  1. Comment style is irregular

    /** 「概览」在各场景下的按钮权限。* */

    The ending * */ is unusual. Prefer:

    /** Button permissions for "Overview" in each scenario. */

    or a standard multi-line JSDoc block.

  2. Verify ./homepage has a default export
    If homepage.js does not use export default, then homepage will be undefined.

  3. The export is nested, not merged

    export default {
      homepage
    }

    is equivalent to:

    export default {
      homepage: homepage
    }

    Consumers must access permissions.homepage....

    If the intention is to expose the permissions directly at the top level, use:

    export default {
      ...homepage
    }

    But only do this if spreading cannot cause key conflicts.

  4. Consider making the config immutable
    If these permissions should not be modified at runtime:

    export default Object.freeze({
      homepage,
    });

    Note that this is only a shallow freeze.

  5. Minor style suggestion
    Add a semicolon after the import/export if your project uses semicolons consistently:

    import homepage from './homepage';
    
    export default {
      homepage,
    };

Suggested corrected version if the nested structure is intended:

/** Button permissions for "Overview" in each scenario. */

import homepage from './homepage';

export default {
  /** Workspace "Overview" permissions. */
  homepage,
};

8 changes: 7 additions & 1 deletion ui/src/permission/modules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import tool from './tool'
import model from './model'
import system from './system'
import trigger from './trigger'
import homepage from './homepage'

export default {
/**
Expand All @@ -36,4 +37,9 @@ export default {
/**
* 「系统全局」按钮权限
* */
system }
system,
/**
* 工作空间「概览」按钮权限
*/
homepage,
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is the review of the provided code snippet:

1. Irregularities & Issues

  • Syntax Breakage (system }): The original line system } was syntactically incorrect in JavaScript/TypeScript object literals unless it used shorthand property syntax correctly on a previous line, but the diff context shows system } immediately followed by + system,.
    • If the original code was ... system }, it likely meant the object ended abruptly or had a syntax error.
    • The fix changes it to system, (adding a comma) which is correct and necessary since a new property (homepage) is added afterward.
  • Missing Import Verification: Ensure that ./homepage actually exists and exports an object compatible with the structure expected elsewhere in this module (likely a button permission definition similar to system, tool, etc.).
  • JSDoc Comment Style Inconsistency:
    • The comment for system uses * */ (note the extra asterisk or spacing issue in some styles, though valid here).
    • The new comment for homepage uses standard JSDoc /** ... */. This is good, but ensure consistency with the rest of the file. The system comment seems to have a slight formatting oddity (* */) in the original context, which might be worth cleaning up if you are refactoring.

2. Optimization Suggestions

  • Tree-Shaking/Bundle Size: If homepage, system, etc., are large modules, ensure your build tool handles side-effect-free imports properly. This looks fine as they are named/default exports from local files.
  • Consistency in Property Definition: If other properties use explicit definitions like key: value, but system and homepage use ES6 shorthand (property instead of key: key), this is valid and modern. No change needed, just be aware.

3. Code Review Verdict

The change is safe and correct assuming:

  1. The file ./homepage exists and exports a default object.
  2. The addition of the comma after system fixes a previous syntax error (if system } was indeed invalid before) or completes the object literal correctly for the new addition.

Recommendation:

  • Verify that ./homepage is implemented.
  • Run a linter to check for any remaining stylistic inconsistencies in comments.

3 changes: 2 additions & 1 deletion ui/src/router/admin/workspace/modules/home.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import perm from '@/permission'
import type { RouteRecordRaw } from 'vue-router'

export const homeRoutes: RouteRecordRaw[] = [
{
path: '',
name: 'workspace-home',
component: () => import('@/views/home/index.vue'),
meta: { title: '概览', activeIcon: 'icon_screen_filled', icon: 'icon_screen_outlined', order: 10 },
meta: { title: '概览', activeIcon: 'icon_screen_filled', icon: 'icon_screen_outlined', order: 10, permission: () => perm.homepage.homepage.read(), next: 'workspace-application',},
},
]
Loading