Repository navigation
refactor: split trigger - #7714
Conversation
| :application="application" | ||
| @closed="handleDialogClosed" | ||
| /> | ||
| </template> |
There was a problem hiding this comment.
Review
Correctness / potential bugs
-
Falsy check on
workspace_idif (!props.application.workspace_id) { ... }
If
workspace_idcan legitimately be0, this wrongly blocks. Prefer an explicit null check:if (props.application.workspace_id == null). -
is_publishtruthiness fordisabled:disabled="!application.is_publish"
Fine if it's a real boolean. If the API can return
0/1or"0"/"1", this is buggy ("0"is truthy). Consider normalizing (Boolean(application.is_publish)) or typing it strictly. Also the property name is grammatically odd —is_publishedreads better. -
Inconsistent return from
handleOpenTriggers
One path returnsundefined, the other returns the promise fromnextTick. This tripsconsistent-returnand is a bit misleading:function handleOpenTriggers(): void { ... dialogMounted.value = true void nextTick(() => resourceTriggerRef.value?.open()) // or: await nextTick(); resourceTriggerRef.value?.open() }
-
Silently swallowing a failed open
resourceTriggerRef.value?.open()does nothing if the ref isn't resolved yet. IfnextTickever isn't enough (async child setup, async component, teleport delay), the click appears to do nothing. Safer alternatives:- Have the child accept an
open-on-mount/v-model:visibleprop instead of imperativeopen()+nextTick, or - Await and guard:
await nextTick(); if (!resourceTriggerRef.value) { MsgError(...) }.
- Have the child accept an
-
State not reset on unexpected unmount
dialogMountedis only reset via@closed. If the child is destroyed by an ancestor (e.g., the dropdown popper is torn down while the dialog is open and the dialog is not teleported),closednever fires and the flag staystrue. ConfirmApplicationTriggerDialogrenders into a teleport/body. If not, this component's placement as a sibling of a dropdown item is fragile — the whole dialog could unmount with the menu. -
Double-click race
Two rapid clicks beforenextTickresolves will callopen()twice. Usually harmless, but guard ifopen()isn't idempotent.
Type-safety issues
-
typeof SomeApias a prop type is likely wrongapi: typeof ResourceTriggerApi
If
ResourceTriggerApiis a class,typeof Xis the constructor type, not an instance. You probably wantInstanceType<typeof ResourceTriggerApi>(the same applies toapplicationApi). If it's a plain object/namespace export, ignore this. -
InstanceType<typeof ApplicationTriggerDialog>only exposesdefineExposed members
With<script setup>,InstanceTypedoes not include internal methods. Make sure the child actually doesdefineExpose({ open }), otherwiseresourceTriggerRef.value?.open()is a type error or a runtime no-op. A cleaner approach is to declare an explicitexport interface ApplicationTriggerDialogExpose { open(): void }and useuseTemplateRef<ApplicationTriggerDialogExpose>('resourceTriggerRef'). -
useTemplateRefrequires Vue ≥ 3.5. Not a problem if you're already on it, but worth confirming for the project's declared peer version.
UX / a11y
- Silent disabled state. When the agent isn't published the menu item is disabled with no explanation. Consider a tooltip ("Publish the agent first") or a disabled reason, otherwise users will think it's broken.
- Hardcoded Chinese error string — route it through your i18n layer if one exists.
Performance / optimization
- Lazy-load the dialog. It's statically imported, so it's in the same chunk even though it's only needed on click:
(You'd then import the expose type separately for
const ApplicationTriggerDialog = defineAsyncComponent(() => import('./ApplicationTriggerDialog.vue'))
useTemplateRef.) This is the biggest win here. - Consider
v-model-driven visibility instead of thev-if+nextTick+ imperativeopen()dance. Keeping the component mounted but its content lazy (<ApplicationTriggerDialog v-model:open="open" />where the inner dialog usesv-ifon its body) removes the timing dependency entirely and is easier to reason about. - Lazy mounting here is otherwise good — you correctly avoid instantiating the dialog until it's needed.
Style nits
dialogMounted→isDialogMounted/showTriggerDialogmatches common Vue naming.- The comment
/* 点击后挂载资源触发器列表。 */documents what, not why; the lazy-mount rationale would be more useful. propsis only used insidesetup, not the template — you could destructure withconst { application } = toRefs(props)if you prefer, but the current form is fine.
| /> | ||
| <!-- MCP 服务配置 --> | ||
| <McpConfigApplicationAction v-if="perm.application.system.mcpConfig()" label="MCP 服务配置" :application="row" /> | ||
| <!-- 查看关联资源 --> |
There was a problem hiding this comment.
Based on the shown diff only, the template syntax itself looks valid. The main points to verify are around the new application-api prop and its availability/type in the child component.
Potential issues
-
Child component must declare
applicationApi- In Vue templates,
:application-api="..."maps to theapplicationApiprop. - If
TriggerApplicationActiondoes not declareapplicationApiin its props, Vue will treat it as a fallthrough attribute. Depending on the component root, this may:- render an unwanted DOM attribute such as
application-api="[object Object]", or - produce a warning / unexpected behavior.
- render an unwanted DOM attribute such as
- Ensure the child has something like:
or make it optional if not always passed.
const props = defineProps<{ api: ... applicationApi: ... application: ... }>()
- In Vue templates,
-
Ensure
SystemResourceApplicationApiis imported/exposed- Since this is inside
<template>,SystemResourceApplicationApimust be available in the component scope. - If using
<script setup>, confirm it is imported:import { SystemResourceApplicationApi } from '...'
- If using Options API, confirm it is returned from
setup, declared indata, or exposed as a computed/property.
- Since this is inside
-
Permission scope may need checking
- The action is still guarded only by
perm.application.system.triggerRead(). - If
TriggerApplicationActionnow usesSystemResourceApplicationApito fetch additional application data, verify thattriggerRead()is sufficient, or whether an application-read permission is also required.
- The action is still guarded only by
-
Prop naming is slightly ambiguous
- Passing both
:apiand:application-apiis understandable, but the names are generic. - Consider clearer names such as:
and in the child:
:trigger-api="SystemResourceTriggerApi" :application-api="SystemResourceApplicationApi"
triggerApi: ... applicationApi: ...
- This improves readability and reduces confusion about which API is used for what.
- Passing both
-
Compatibility with existing usages
- If
applicationApiis required in the child, every existing usage ofTriggerApplicationActionmust pass it. - If some usages do not, make the prop optional and handle the missing value:
applicationApi?: ApplicationApiType
- If
Optimization suggestions
- If the child component can reliably import
SystemResourceApplicationApiitself, passing it as a prop may be unnecessary and adds coupling. However, passing it as a prop is useful for testability and dependency injection. - If many action components receive similar API objects, consider centralizing API injection or using a shared composable/provider rather than passing multiple API props through every component.
- Ensure the API objects are stable constants. If they are ever stored in reactive state, consider
markRawto avoid unnecessary Vue proxying of class/API objects.
Overall: the diff is syntactically fine, but the key thing to check is that TriggerApplicationAction explicitly declares and correctly types the new applicationApi prop, and that SystemResourceApplicationApi is properly imported/exposed.
refactor: split trigger
【【资源授权】角色未授予「编辑触发器」权限,触发器编辑页面底部「保存」按钮未置灰/隐藏】
https://www.tapd.cn/tapd_fe/45245355/bug/detail/1145245355001078674