[shaohuzhang1]

feat: trigger permission

[shaohuzhang1]

Here is the review of the provided code diff, highlighting potential issues and optimization suggestions:

1. Critical Logic Issue: props Accessibility in Helper Function

The applicationTriggerPermission function references props.application.id. However, in Vue <script setup>, functions defined outside the script setup context (or in certain utility scopes) might not have direct access to props unless they are defined correctly within the component setup or closed over properly.

• Risk: If props is not explicitly passed into applicationTriggerPermission or if TypeScript/ESLint complains about implicit dependency, this could break.
• Fix: Ensure props is accessible via closure (which it likely is since it's inside .vue file script setup), but consider making props an explicit parameter for clarity and testability:

  function applicationTriggerPermission(
    method: ApplicationTriggerPermissionMethod, 
    appId: string // Explicitly pass ID if needed for external calls
  ): boolean { ... }

  Or better, keep it as is but ensure props is initialized before this function is ever called. Since defineProps returns reactive props, accessing props.application.id inside a helper function defined after defineProps is standard and safe.

2. Performance & Reactive Updates

• Issue: v-if="applicationTriggerPermission('...')" is evaluated on every render cycle. While Boolean checks are cheap, isSystemResource() and perm[...]() might involve non-trivial logic.
• Suggestion: If permissions don’t change dynamically during the component’s lifecycle, compute these values once using computed:

  const canCreate = computed(() => applicationTriggerPermission('triggerCreate'))
  const canEdit = computed(() => applicationTriggerPermission('triggerEdit'))
  const canDelete = computed(() => applicationTriggerPermission('triggerDelete'))

  Then use v-if="canEdit" and v-if="canDelete" in the template for each trigger card.

  ⚠️ Note: v-if inside a v-for loop (if applicable) will re-evaluate per item. Using computed helps avoid redundant function calls.

3. UX Consistency: Hidden vs. Disabled Buttons

• Current Behavior: Buttons are hidden (v-if) when permission is denied.
• Alternative: Some UX patterns prefer disabling buttons (:disabled="!applicationTriggerPermission(...)") instead of hiding them. Hiding can make the UI feel inconsistent or "broken" if users expect controls to be visible but inactive.
• Recommendation: Confirm with your product team whether hidden or disabled buttons are preferred for this application. If accessibility matters, aria-disabled attributes may also be needed.

4. Type Safety & Enum Usage

• The type ApplicationTriggerPermissionMethod is well-defined and restricts allowed strings. ✅ Good practice.
• Ensure that perm.application.system and perm.application.workspace objects actually contain methods named exactly 'triggerCreate', 'triggerEdit', 'triggerDelete'. Mismatched names will result in runtime errors (though TypeScript should catch this if types are strict).

5. Security & Backend Validation

• ⚠️ Important: Front-end permission checks (v-if) are for UX only. They do not provide security.
• Ensure that API calls (handleOpenForm, handleRemoveTrigger) are also protected by backend authentication/authorization checks. Never rely solely on the UI hiding buttons.

6. Code Style & Naming

• The helper function applicationTriggerPermission is good, but consider naming it hasTriggerPermission for better readability.
• Import computed from vue if you adopt the optimization suggestion above.

---

✅ Recommended Refactored Code Snippet (with computed)

import { computed } from 'vue'
// ... other imports

type ApplicationTriggerPermissionMethod = 'triggerCreate' | 'triggerEdit' | 'triggerDelete'

function hasTriggerPermission(method: ApplicationTriggerPermissionMethod): boolean {
  const appId = props.application?.id // Optional chaining for safety
  if (!appId && !isSystemResource()) return false // Guard against undefined app ID
  
  if (isSystemResource()) {
    return Boolean(perm.application.system[method]?.())
  }
  
  return Boolean(perm.application.workspace[method]?.(appId))
}

// Compute once per component instance
const canCreateTrigger = computed(() => hasTriggerPermission('triggerCreate'))
const canEditTrigger = computed(() => hasTriggerPermission('triggerEdit'))
const canDeleteTrigger = computed(() => hasTriggerPermission('triggerDelete'))

And in the template:

<el-button v-if="canCreateTrigger" text :disabled="loading" @click="handleOpenForm()" class="text-N900!">
  <!-- ... -->
</el-button>

<!-- Inside v-for -->
<el-button v-if="canEditTrigger" text :disabled="loading" @click="handleOpenForm(trigger.id)">
  <!-- ... -->
</el-button>
<el-button v-if="canDeleteTrigger" text :disabled="loading" @click="handleRemoveTrigger(trigger)">
  <!-- ... -->
</el-button>

This approach improves performance, readability, and guards against potential null/undefined errors.

[shaohuzhang1]

Review

1. props used before declaration (potential bug)

toolTriggerPermission() is defined before const props = defineProps<...>(), and it references props.tool.id. This works at runtime only because the function is called later (after setup completes). But:

• It's a hoisting/ordering smell.
• If toolTriggerPermission were ever called during setup (or if code is refactored), props would be in TDZ.

Fix: Move the function below defineProps.

2. toolTriggerPermission is not reactive

The permission result depends on:

• isSystemResource() (likely a global/config state)
• perm.tool.system/workspace (permission store)
• props.tool.id

If any of these can change at runtime (e.g., props change, permission store updates), the v-if won't re-evaluate because the function isn't tracked as reactive. Vue tracks reactivity by reading reactive sources during render. Calling a plain function that reads perm may or may not track depending on whether perm is a reactive/ref. In many permission implementations it's a plain object — then no tracking occurs.

Recommended fix — use a computed:

import { computed } from 'vue'

const triggerPermissions = computed(() => {
  const system = isSystemResource()
  return {
    create: system ? !!perm.tool.system.triggerCreate() : !!perm.tool.workspace.triggerCreate(props.tool.id),
    edit:   system ? !!perm.tool.system.triggerEdit()   : !!perm.tool.workspace.triggerEdit(props.tool.id),
    delete: system ? !!perm.tool.system.triggerDelete() : !!perm.tool.workspace.triggerDelete(props.tool.id),
  }
})

Then in template: v-if="triggerPermissions.create", etc.

This also avoids calling three separate functions per render item.

3. Boolean(...) vs !!

Minor style nit — Boolean(perm...) is fine but !! is more idiomatic in TS/Vue codebases. Not an issue.

4. Repeated calls per trigger row

With N triggers, toolTriggerPermission('edit') and 'delete' are each invoked N times per render. The computed above reduces this to one evaluation cached until deps change — better performance.

5. Type union literal lacks exhaustiveness safety

If new methods are added to ToolTriggerPermissionMethod, there's no compile-time guarantee perm.tool.system[method] exists. Consider using keyof typeof perm.tool.system (or a mapped type) so additions stay type-safe. Also, if system and workspace don't share the same keys/method signatures, indexing with a shared union will be unsound.

6. Silent denial vs. disabled state

You switched from :disabled="loading" to hiding buttons entirely (v-if). Preserve loading feedback: when a user does have permission but something's loading, they'll see the button vanish? No — v-if combines both conditions? Actually you removed :disabled implications only partially — check: v-if="toolTriggerPermission('triggerCreate')" still has :disabled="loading", so it's fine. Just confirm intentional UX choice of hiding vs disabling for unauthorized users.

7. Handle empty-state UI

If a user has no triggerCreate permission, the "添加" button disappears — the header <h4>触发器</h4> may look bare with no action. Consider conditional rendering of the whole action group or an empty-state hint.

8. Assumption about props.tool.id being defined

toolTriggerPermission accesses props.tool.id without guarding for undefined. Presumably tool is always provided, but worth confirming since hooks into workspace.trigger* may throw on undefined id.

9. Testing / mocking risk

Importing perm and isSystemResource directly makes the component harder to unit test. Consider injecting via provide/inject or composable (useToolPermissions(toolId)) which can be mocked — improves testability and reusability if other components need the same logic.

Summary of priorities:

1. ⚠️ Make permission check reactive (computed).
2. ⚠️ Move function after defineProps.
3. ✅ Cache results to avoid repeated calls per row.
4. ✅ Consider testing/modularity (extract to composable).
5. 💡 Type-safety for the method key.