Skip to content
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { computed, ref, watch } from 'vue'
import RoleApi from '@/api/admin/system/role'
import type { RoleItem, RolePermission, RolePermissionModule, RolePermissionModuleGroup } from '@/api/types'
import { MsgSuccess } from '@/utils/message'
import perm from '@/permission';

interface PermissionTableRow {
id: string
Expand Down Expand Up @@ -185,7 +186,7 @@ watch(() => props.currentRole.id, loadPermissions, { immediate: true })
</MkTable>

<footer v-if="!disabled" class="sticky -mb-6 -ml-6 -mr-6 bottom-0 z-10 mt-auto flex shrink-0 justify-end border-t bg-white px-6 py-4">
<el-button type="primary" :loading="loading" @click="handleSave">保存</el-button>
<el-button v-if="perm.system.role.edit()" type="primary" :loading="loading" @click="handleSave">保存</el-button>
</footer>
</div>
</template>

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 analysis of the code changes with potential issues and optimization suggestions:

🔴 Critical Issue: Invalid Permission Syntax

Problem:
v-if="perm.system.role.edit()" is invalid JavaScript syntax inside a Vue directive. You are calling the function immediately during template rendering, which likely returns undefined or throws an error, rather than checking a permission boolean/computed property.

Why it fails:

  • perm.system.role.edit() attempts to invoke a method. If edit is a function that returns a boolean or permission object, this might work syntactically but is not standard practice for reactive permission checks.
  • More importantly, if perm is a module/object, perm.system.role.edit is likely a reference to a function or a getter, not the result of the check. Vue directives expect an expression evaluating to truthy/falsy, not a function call unless intentionally done (which is rare and discouraged in templates).

✅ Fix Options:

  1. If perm.system.role.edit is a computed/reactive boolean getter:

    - <el-button v-if="perm.system.role.edit()" ...
    + <el-button v-if="perm.system.role.edit" ...
  2. If you need to call a function that returns a boolean (e.g., hasPermission('role.edit')):

    <!-- Prefer computed props over inline function calls -->
    <script setup>
    import perm from '@/permission';
    const canEdit = computed(() => perm.canEditRole()); // Example
    </script>
    <template>
      <el-button v-if="canEdit" ... >
    </template>
  3. Best Practice: Use a Computed Property
    Inline expressions in templates should be kept simple. Move logic to <script>:

    import perm from '@/permission'
    
    const canSaveRoles = computed(() => perm.hasPermission('role.edit')) // adapt to your perm API
    <el-button v-if="canSaveRoles" type="primary" :loading="loading" @click="handleSave">保存</el-button>

🟡 Optimization & Style Suggestions

1. Avoid Trailing Semicolon in Import

Minor style issue:

- import perm from '@/permission';
+ import perm from '@/permission'

Most Vue/JS projects use ESLint/Prettier rules that discourage trailing semicolons on imports. Consistency matters.

2. Sticky Footer CSS Concerns

The footer uses sticky, -mb-6 -ml-6 -mr-6, and mt-auto. Ensure:

  • The parent container has enough height and correct flex column layout for mt-auto and sticky positioning to behave predictably.
  • Negative margins combined with sticky can cause overflow/layout bugs in some browsers. Test thoroughly, especially when content scrolls.

3. Accessibility / UX Improvement

If permission is denied, the button disappears entirely (v-if). Consider:

  • Disabling it instead using :disabled="!canSaveRoles" so users know the action exists but requires higher privileges.
  • Or show a tooltip/help icon explaining why they cannot save.
<el-button 
  v-if="canSaveRoles" 
  type="primary" 
  :loading="loading" 
  @click="handleSave"
>
  保存
</el-button>
<!-- Alternative: disable visually -->
<!-- <el-button v-if="isAdmin" :disabled="!canSaveRoles" type="primary" ...> -->

✅ Corrected Code Example

// In <script setup lang="ts">
import { computed } from 'vue'
import perm from '@/permission'

const canSavePermissions = computed(() => {
  // Adjust based on actual permission API structure
  return perm.check ? perm.check('system:role:edit') : false 
  // OR if using method-style:
  // return typeof perm.isAllowed === 'function' ? perm.isAllowed('edit-role') : false
})
<footer v-if="!disabled" class="...">
  <el-button 
    v-if="canSavePermissions" 
    type="primary" 
    :loading="loading" 
    @click="handleSave"
  >
    保存
  </el-button>
</footer>

Final Summary

Category Status Recommendation
Bug 🔴 High Fix invalid function call in v-if; use reactive/computed value.
Style 🟡 Low Remove trailing semicolon; move logic to computed prop.
UX 🟢 Optional Disable button instead of hiding if user needs visibility.
CSS 🟢 OK Verify sticky+negative margin behavior across browsers.

Expand Down
Loading