Skip to content

feat: system and workspace homepage permission - #7867

Merged
peslyn merged 1 commit into
v3from
pr@v3@feat_system_and_workspace_homepage_permission
Nov 12, 2026
Merged

peslyn merged 1 commit into
v3from
pr@v3@feat_system_and_workspace_homepage_permission

Conversation

@shaohuzhang1

Copy link
Copy Markdown
Contributor

feat: system and workspace homepage permission

<el-button v-if="canExport" plain :loading="exporting" :disabled="loading" @click="handleExport">
<MkIcon name="icon_export_outlined" />
<span>导出</span>
</el-button>

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 an analysis of the provided code diff, focusing on potential issues and optimization suggestions:

1. Permission Logic Duplication & Maintainability

  • Issue: The permission checks perm.system.homepage.export() and perm.homepage.homepage.export() are hardcoded in a ternary operator within the component.
  • Risk: If more scopes or distinct permission paths are added later, this computed property will become cluttered and harder to maintain.
  • Suggestion: Consider extracting the permission resolution logic into a centralized utility or a composable (e.g., useExportPermission(scope: string)) if this pattern repeats across components. Alternatively, ensure your perms object structure supports a unified method like perm.getExport(scope) to simplify the call.

2. Reactive Dependency on route.meta

  • Context: route is obtained via useRoute(). In Vue Router 4, useRoute() returns a reactive reference. Accessing route.meta.scope inside a computed property is correctly reactive; it will re-evaluate when the route meta changes.
  • Note: Ensure that route.meta.scope is always defined as 'system' for system home pages. If scope can be undefined, isSystemHome will evaluate to false correctly, which is safe but worth verifying against your router configuration.

3. Redundant Boolean Conversion

  • Code: Boolean(perm.system.homepage.export())
  • Optimization: In JavaScript/TypeScript, most permission functions already return a boolean or a truthy/falsy value. Wrapping them in Boolean() is redundant unless you have a specific reason to coerce non-boolean values (like strings or empty arrays). It adds visual noise without significant functional benefit. You can likely simplify to:
    const canExport = computed(() =>
      isSystemHome.value ? perm.system.homepage.export() : perm.homepage.homepage.export(),
    )

4. Vue Composition API Best Practice

  • Positive: Using computed for canExport and isSystemHome is correct. It ensures these values are only recalculated when their dependencies (route.meta.scope) change.
  • Import Clarity: import { computed } from 'vue' was added. Ensure you aren't duplicating imports if computed was already imported elsewhere in the <script setup> block (common in Vue 3 + Vite with auto-imports, but explicit is fine too).

5. UI Accessibility & UX

  • Observation: The export button is now conditionally rendered with v-if="canExport".
  • Suggestion: Depending on your design guidelines, consider using v-disabled (if available in your UI library) or disabling the button with a tooltip instead of hiding it entirely. This provides better feedback to users who know they should have access but lack permissions, rather than making the feature seem "missing." However, v-if is perfectly valid if the feature should be strictly invisible to unauthorized users.

Final Recommendation Code Snippet

If you accept the optimization in point #3:

const route = useRoute();
const isSystemHome = computed(() => route.meta.scope === 'system');

// Simplified permission check
const canExport = computed(() =>
  isSystemHome.value
    ? perm.system.homepage.export() 
    : perm.homepage.homepage.export(),
);

Verdict: The code is functionally sound and follows Vue 3 Composition API patterns. The primary improvement is reducing boilerplate in the permission check and considering future scalability for permission logic.

*/
edit: () => can(P.HOMEPAGE_EXPORT)
export: () => 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 primary irregularity is a mismatch between the code change and the comment.

  1. Comment Inaccuracy: The comment 工作空间「概览」导出权限 translates to "Workspace 'Overview' Export Permission". However, the property key was changed from edit to export, which aligns with the comment, but it suggests that the comment itself may be outdated if the function's purpose has shifted. More likely, the comment accurately describes the intent, and the key rename fixes a logical consistency issue (the code now correctly exposes an export permission).
  2. Potential Naming Conflict: If this object is merged with others or used in a context where export is a reserved keyword (though unlikely in standard JS objects as it's not a reserved word for property names), consider context. However, export is valid as a property name.
  3. Type Checking: Ensure P.HOMEPAGE_EXPORT is consistently defined and that export is recognized elsewhere in the application logic (e.g., in permission checks, UI components, or router guards) as the new expected key. If the previous key edit was referenced elsewhere, those references will break.

Optimization/Recommendation:

  • Verify all consumers of this permission object use the new export key instead of edit.
  • Update any documentation or tests referencing the old edit key for this specific permission.
  • Consider if other permissions in this module follow a consistent naming convention (e.g., view, edit, delete, export) and ensure full consistency.

@peslyn
peslyn merged commit a32cbbb into v3 Nov 12, 2026
3 of 4 checks passed
@peslyn
peslyn deleted the pr@v3@feat_system_and_workspace_homepage_permission branch November 12, 2026 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants