Skip to content

fix: support click model card to edit - #7870

Merged
fit2cloudwxx merged 1 commit into
v3from
pr@v3@fix_supportClickModelCardEdit
Nov 12, 2026
Merged

fit2cloudwxx merged 1 commit into
v3from
pr@v3@fix_supportClickModelCardEdit

Conversation

@shaohuzhang1

@shaohuzhang1 shaohuzhang1 commented Nov 12, 2026 •

Copy link
Copy Markdown
Contributor

【【模型】共享卡片无交互状态,参照工具】
https://www.tapd.cn/tapd_fe/45245355/bug/detail/1145245355001079306

<EditModelDrawer v-if="editDrawerMounted" ref="editModelDrawerRef" :api="ModelApi" @closed="editDrawerMounted = false" @refresh="loadModels" />
</template>

<style scoped lang="scss"></style>

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 diff is structurally reasonable: it replaces a per-card edit action with one shared EditModelDrawer, and the editDrawerMounted guard prevents duplicate drawer mounts. However, there are several issues to verify/fix.

1. Event bubbling is the main risk

<ModelCard
  ...
  @click="handleOpenEditModel(model)"
>

Adding @click to the whole card will also receive clicks from interactive children inside the card, especially the action-dropdown trigger and any buttons/icons. That means:

  • clicking the ... / action menu trigger may also open the edit drawer;
  • clicking other dropdown actions may trigger the card edit handler if the dropdown is not teleported or does not stop propagation;
  • a single click may cause two actions.

Fix by either:

  • removing the whole-card click and using a dedicated edit button/area, or
  • adding @click.stop to the action-dropdown trigger and all interactive controls inside ModelCard.

Also verify that ModelCard has a single root element. If it is a fragment/multi-root component, the native click listener may not fall through as expected.

2. Verify the EditModelDrawer contract

The new code assumes that EditModelDrawer:

  • exposes an open(provider, model) method via defineExpose;
  • emits closed;
  • emits refresh;
  • accepts the api prop.

If any of these is missing or named differently, the drawer may never open, never unmount, or never refresh the list.

If the drawer uses v-model / update:modelValue instead of closed, then editDrawerMounted will never be reset. That would permanently block future edit attempts.

3. editDrawerMounted can get stuck

editDrawerMounted.value = true
await nextTick()
editModelDrawerRef.value?.open(getModelProvider(model), model)

If the ref is null, or if open() throws, editDrawerMounted remains true, and all future edit clicks are ignored. Use a safer version:

async function handleOpenEditModel(model: ModelItem) {
  if (
    isShared.value ||
    !perm.model.workspace.modify(model.id) ||
    editDrawerMounted.value
  ) {
    return
  }

  editDrawerMounted.value = true
  await nextTick()

  const drawer = editModelDrawerRef.value
  if (!drawer) {
    editDrawerMounted.value = false
    return
  }

  try {
    await drawer.open(getModelProvider(model), model)
  } catch (error) {
    editDrawerMounted.value = false
    throw error
  }
}

If open() is synchronous, the try/catch is still useful; await can be removed if it is not async.

4. useTemplateRef requires Vue 3.5+

const editModelDrawerRef = useTemplateRef<
  InstanceType<typeof EditModelDrawer>
>('editModelDrawerRef')

This is fine on Vue 3.5+, but will not work on older Vue 3 versions. Confirm the project version. If the project is on Vue < 3.5, use:

const editModelDrawerRef = ref<InstanceType<typeof EditModelDrawer> | null>(null)

5. Verify MkAction is available

<MkAction
  v-if="perm.model.workspace.modify(model.id)"
  label="编辑"
  icon="icon_edit_outlined"
  @click="handleOpenEditModel(model)"
/>

Since EditModelAction was removed, verify that MkAction is globally registered or auto-imported. Also verify that it accepts the icon prop and that clicking it closes the dropdown as expected.

6. Accessibility / UX concern

A whole card with @click is not keyboard-accessible unless it has proper role, tabindex, and keydown.enter / keydown.space handling. A dedicated edit button inside the dropdown is safer and clearer. At minimum, avoid making the card look clickable when the user lacks modify permission or when isShared is true.

7. Minor optimization

The formatting-only changes to ButtonAddModel, ParamSettingAction, and RelatedResourcesModelAction increase diff noise. They are harmless, but can be omitted to keep the review focused.

Summary

The biggest issue is the whole-card @click combined with an action dropdown inside the card. Add @click.stop on interactive card children or use a dedicated edit trigger. Then verify the drawer exposes open, emits closed/refresh, and that editDrawerMounted is reset if opening fails.

@fit2cloudwxx
fit2cloudwxx merged commit 0222625 into v3 Nov 12, 2026
3 of 4 checks passed
@fit2cloudwxx
fit2cloudwxx deleted the pr@v3@fix_supportClickModelCardEdit branch November 12, 2026 09:26
>
<template #icon>
<span class="block h-6 w-6" :innerHTML="provider.icon" />
</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 an analysis of the code diff provided:

1. Potential Issue: HTML Injection Risk

Location: <template> section

<span class="block h-6 w-6" :innerHTML="provider.icon" />

Issue: Using v-html (or its shorthand :innerHTML) renders raw HTML. If provider.icon contains untrusted user input or comes from a database without strict sanitization, this creates a Cross-Site Scripting (XSS) vulnerability.
Suggestion: Ensure provider.icon is strictly sanitized or use an SVG component/icon library instead of injecting raw HTML strings.

2. Typo/Vue 3 API Consistency

Location: <script setup> and <template>
Issue: The code uses innerHTML. In Vue templates with v-bind, you should use innerHTML (DOM property) which works, but it's less common than using v-html. However, the bigger "irregularity" is stylistic.
Note: While :innerHTML works, standard Vue practice for dynamic HTML is v-html="...". If you stick with binding, ensure your linter allows it. More importantly, checking if MkResourceCard actually handles the @click event properly is crucial.

3. Missing Prop Definition for disabled

Location: Template binding :disabled="disabled"
Issue: You are passing :disabled="disabled" to MkResourceCard, but looking at defineProps in the snippet, disabled is not shown as being defined in props.
Check: Ensure disabled is explicitly declared in defineProps<{ ... }>(). If it’s not defined, Vue will treat it as an attr (fallthrough) instead of a prop, and TypeScript may complain if strict checking is enabled.
Current snippet shows:

const props = defineProps<{
   shared: boolean
}>()

Fix: Add disabled: boolean to the props interface if used.

4. Event Type Optimization

Location: defineEmits

const emit = defineEmits<{ click: [event: MouseEvent] }>()

Observation: This is a good, modern Vue 3 pattern for type-safe emits. No issues here.

5. Logic Check: handleClick Guard

Location: Script logic

function handleClick(event: MouseEvent) {
  if (props.disabled) return
  emit('click', event)
}

Optimization/Suggestion:
While guarding inside the handler prevents the event from propagating in logic, many UI component libraries (like Element Plus, Ant Design Vue, etc.) automatically ignore clicks when disabled is true on root elements or apply pointer-events: none via CSS.

  • If MkResourceCard handles disabled state internally: Your JS guard might be redundant but safe.
  • If UI consistency is priority: Ensure that visual styling (cursor changes, opacity) also reflects the disabled state. Relying solely on JS prevention while the element looks clickable can lead to poor UX.

Summary of Recommendations

  1. Security: Verify provider.icon source. If external/untrusted, sanitize it or avoid innerHTML.
  2. Completeness: Confirm disabled is included in defineProps.
  3. UI/UX: Ensure MkResourceCard has appropriate visual feedback for the disabled state (e.g., opacity, cursor-not-allowed).
  4. Vue Best Practice: Consider using v-html="provider.icon" instead of :innerHTML for better readability and standard template syntax recognition by tools/linters.

Revised Template Snippet (for style):

<span class="block h-6 w-6" v-html="provider.icon" />

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