Repository navigation
fix: support click model card to edit - #7870
Conversation
| <EditModelDrawer v-if="editDrawerMounted" ref="editModelDrawerRef" :api="ModelApi" @closed="editDrawerMounted = false" @refresh="loadModels" /> | ||
| </template> | ||
|
|
||
| <style scoped lang="scss"></style> |
There was a problem hiding this comment.
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.stopto the action-dropdown trigger and all interactive controls insideModelCard.
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 viadefineExpose; - emits
closed; - emits
refresh; - accepts the
apiprop.
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.
| > | ||
| <template #icon> | ||
| <span class="block h-6 w-6" :innerHTML="provider.icon" /> | ||
| </template> |
There was a problem hiding this comment.
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
MkResourceCardhandles 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
- Security: Verify
provider.iconsource. If external/untrusted, sanitize it or avoidinnerHTML. - Completeness: Confirm
disabledis included indefineProps. - UI/UX: Ensure
MkResourceCardhas appropriate visual feedback for thedisabledstate (e.g.,opacity,cursor-not-allowed). - Vue Best Practice: Consider using
v-html="provider.icon"instead of:innerHTMLfor 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" />
【【模型】共享卡片无交互状态,参照工具】
https://www.tapd.cn/tapd_fe/45245355/bug/detail/1145245355001079306