Skip to content

refactor: split trigger - #7714

Merged
fit2cloudwxx merged 1 commit into
v3from
pr@v3@refactor_splitTrigger
Nov 6, 2026
Merged

fit2cloudwxx merged 1 commit into
v3from
pr@v3@refactor_splitTrigger

Conversation

@shaohuzhang1

@shaohuzhang1 shaohuzhang1 commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

refactor: split trigger
【【资源授权】角色未授予「编辑触发器」权限,触发器编辑页面底部「保存」按钮未置灰/隐藏】
https://www.tapd.cn/tapd_fe/45245355/bug/detail/1145245355001078674

:application="application"
@closed="handleDialogClosed"
/>
</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.

Review

Correctness / potential bugs

  1. Falsy check on workspace_id

    if (!props.application.workspace_id) { ... }

    If workspace_id can legitimately be 0, this wrongly blocks. Prefer an explicit null check: if (props.application.workspace_id == null).

  2. is_publish truthiness for disabled

    :disabled="!application.is_publish"

    Fine if it's a real boolean. If the API can return 0/1 or "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_published reads better.

  3. Inconsistent return from handleOpenTriggers
    One path returns undefined, the other returns the promise from nextTick. This trips consistent-return and is a bit misleading:

    function handleOpenTriggers(): void {
      ...
      dialogMounted.value = true
      void nextTick(() => resourceTriggerRef.value?.open())
      // or: await nextTick(); resourceTriggerRef.value?.open()
    }
  4. Silently swallowing a failed open
    resourceTriggerRef.value?.open() does nothing if the ref isn't resolved yet. If nextTick ever 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:visible prop instead of imperative open() + nextTick, or
    • Await and guard: await nextTick(); if (!resourceTriggerRef.value) { MsgError(...) }.
  5. State not reset on unexpected unmount
    dialogMounted is 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), closed never fires and the flag stays true. Confirm ApplicationTriggerDialog renders 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.

  6. Double-click race
    Two rapid clicks before nextTick resolves will call open() twice. Usually harmless, but guard if open() isn't idempotent.

Type-safety issues

  1. typeof SomeApi as a prop type is likely wrong

    api: typeof ResourceTriggerApi

    If ResourceTriggerApi is a class, typeof X is the constructor type, not an instance. You probably want InstanceType<typeof ResourceTriggerApi> (the same applies to applicationApi). If it's a plain object/namespace export, ignore this.

  2. InstanceType<typeof ApplicationTriggerDialog> only exposes defineExposed members
    With <script setup>, InstanceType does not include internal methods. Make sure the child actually does defineExpose({ open }), otherwise resourceTriggerRef.value?.open() is a type error or a runtime no-op. A cleaner approach is to declare an explicit export interface ApplicationTriggerDialogExpose { open(): void } and use useTemplateRef<ApplicationTriggerDialogExpose>('resourceTriggerRef').

  3. useTemplateRef requires Vue ≥ 3.5. Not a problem if you're already on it, but worth confirming for the project's declared peer version.

UX / a11y

  1. 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.
  2. Hardcoded Chinese error string — route it through your i18n layer if one exists.

Performance / optimization

  1. Lazy-load the dialog. It's statically imported, so it's in the same chunk even though it's only needed on click:
    const ApplicationTriggerDialog = defineAsyncComponent(() => import('./ApplicationTriggerDialog.vue'))
    (You'd then import the expose type separately for useTemplateRef.) This is the biggest win here.
  2. Consider v-model-driven visibility instead of the v-if + nextTick + imperative open() dance. Keeping the component mounted but its content lazy (<ApplicationTriggerDialog v-model:open="open" /> where the inner dialog uses v-if on its body) removes the timing dependency entirely and is easier to reason about.
  3. Lazy mounting here is otherwise good — you correctly avoid instantiating the dialog until it's needed.

Style nits

  1. dialogMounted → isDialogMounted / showTriggerDialog matches common Vue naming.
  2. The comment /* 点击后挂载资源触发器列表。 */ documents what, not why; the lazy-mount rationale would be more useful.
  3. props is only used inside setup, not the template — you could destructure with const { application } = toRefs(props) if you prefer, but the current form is fine.

/>
<!-- MCP 服务配置 -->
<McpConfigApplicationAction v-if="perm.application.system.mcpConfig()" label="MCP 服务配置" :application="row" />
<!-- 查看关联资源 -->

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.

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

  1. Child component must declare applicationApi

    • In Vue templates, :application-api="..." maps to the applicationApi prop.
    • If TriggerApplicationAction does not declare applicationApi in 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.
    • Ensure the child has something like:
      const props = defineProps<{
        api: ...
        applicationApi: ...
        application: ...
      }>()
      or make it optional if not always passed.
  2. Ensure SystemResourceApplicationApi is imported/exposed

    • Since this is inside <template>, SystemResourceApplicationApi must 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 in data, or exposed as a computed/property.
  3. Permission scope may need checking

    • The action is still guarded only by perm.application.system.triggerRead().
    • If TriggerApplicationAction now uses SystemResourceApplicationApi to fetch additional application data, verify that triggerRead() is sufficient, or whether an application-read permission is also required.
  4. Prop naming is slightly ambiguous

    • Passing both :api and :application-api is understandable, but the names are generic.
    • Consider clearer names such as:
      :trigger-api="SystemResourceTriggerApi"
      :application-api="SystemResourceApplicationApi"
      and in the child:
      triggerApi: ...
      applicationApi: ...
    • This improves readability and reduces confusion about which API is used for what.
  5. Compatibility with existing usages

    • If applicationApi is required in the child, every existing usage of TriggerApplicationAction must pass it.
    • If some usages do not, make the prop optional and handle the missing value:
      applicationApi?: ApplicationApiType

Optimization suggestions

  • If the child component can reliably import SystemResourceApplicationApi itself, 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 markRaw to 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.

@fit2cloudwxx
fit2cloudwxx merged commit 92fd0e8 into v3 Nov 6, 2026
3 of 4 checks passed
@fit2cloudwxx
fit2cloudwxx deleted the pr@v3@refactor_splitTrigger branch November 6, 2026 07:46
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