Skip to content

perf: style #7868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wangdan-fit2cloud merged 1 commit into from
Nov 12, 2026
Merged

perf: style #7868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ui/src/conversation-panel/view/portal/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,5 +107,15 @@ export function createPortalConversation(options: PortalConversationOptions): Po
bundle.list.conversations.value = [...(findGroup(applicationId)?.conversations ?? [])]
})

// 当前智能体从无历史变为有会话时自动展开，后续消息不覆盖用户手动收起的状态。
watch(
() => [currentApplicationId.value, bundle.list.conversations.value.length] as const,
([applicationId, count], [previousApplicationId, previousCount]) => {
if (applicationId !== previousApplicationId || previousCount !== 0 || count === 0) return
const group = portalList.applicationGroups.value.find((applicationGroup) => applicationGroup.id === applicationId)
if (group) group.expanded = true
},
)

return { ...bundle, portalList }
}

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

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.

No syntax errors are apparent, and the watcher generally implements the intended “same application: 0 → >0 conversations, auto-expand once” behavior. The previousCount !== 0 guard correctly prevents later message-count changes from overriding a manual collapse.

Potential issues / suggestions:

  1. Missed auto-expand when app ID and count change in the same flush

    • If currentApplicationId changes and the new app already has conversations in the same tick, the guard applicationId !== previousApplicationId returns early, so the group is not expanded.
    • This may be intentional, but if switching to an app with existing conversations should also expand it, the logic needs adjustment.

  2. No retry if the group is not found

    • If portalList.applicationGroups.value has not been updated/loaded yet, find(...) returns undefined, and the expansion is permanently skipped because the count transition is already consumed.
    • Consider { flush: 'post' } or nextTick() if group availability can lag behind conversation changes.

  3. Manual collapse can be overridden after conversations become empty

    • Scenario: user manually collapses a group with 1 conversation → all conversations are deleted (count = 0) → a new conversation arrives (count = 1) → watcher expands again.
    • If manual collapse must survive this case, track an explicit per-group userCollapsed/autoExpandDone flag instead of relying only on previousCount === 0.

  4. Direct mutation of group.expanded

    • group.expanded = true assumes the group object is deeply reactive/mutable.
    • If applicationGroups is a shallowRef, readonly, frozen, or computed structure, this mutation may not trigger UI updates. Prefer a centralized action such as portalList.expandGroup(applicationId).

  5. Watcher cleanup

    • If createPortalConversation can be called outside a component setup scope, the watcher may not be automatically disposed. Verify lifecycle handling or dispose it explicitly.

  6. No immediate

    • If an application is created with conversations already present, the group will not auto-expand. If that is desired, add immediate with explicit initialization logic.

Optimization suggestion:

Use separate watch sources instead of constructing a new tuple array on each evaluation. This is slightly clearer and avoids unnecessary tuple allocation:

watch(
  [currentApplicationId, () => bundle.list.conversations.value.length] as const,
  ([applicationId, count], [previousApplicationId, previousCount]) => {
    if (applicationId !== previousApplicationId) return
    if (previousCount !== 0 || count === 0) return

    const group = portalList.applicationGroups.value.find(
      (applicationGroup) => applicationGroup.id === applicationId,
    )

    if (group) group.expanded = true
  },
  { flush: 'post' },
)

Overall: the logic is acceptable for the stated narrow case, but it is fragile around group-loading timing and manual-collapse persistence. I’d recommend using a flush: 'post' watcher and, if manual collapse must be respected across empty/non-empty transitions, storing explicit expansion state per group.

5 changes: 5 additions & 0 deletions ui/src/styles/STYLE_README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,11 @@ Element Plus 覆盖、主题变量和应用样式。Chat 入口还显式引入 E
模板无需重复添加 `transition-colors` 或 `focus-within:border-primary`。
生成输入区使用 `mk-input-box`，不叠加对话专属类。

## 带头像的名称表单

32px 头像与名称输入框以 `gap-3` 横向排列时，在所属 `el-form-item` 上添加
`mk-form-item-with-avatar`，由 `element-plus.scss` 统一让校验提示与输入框左边缘对齐。

## 连体表单控件

`app.scss` 中的 `mk-input-group` 用于默认尺寸的两段式控件组合，统一横向布局、外边框、
Expand Down
10 changes: 9 additions & 1 deletion ui/src/styles/element-plus.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,9 @@
/* form */
.el-form-item {
margin-bottom: calc(var(--spacing) * 4);
&.mk-form-item-with-avatar > .el-form-item__content > .el-form-item__error {
padding-left: calc(32px + var(--spacing) * 3);
}
&.small {
margin-bottom: calc(var(--spacing) * 2);
}
Expand Down Expand Up @@ -1146,7 +1149,8 @@
.el-table {
--el-table-header-bg-color: var(--mk-N100);
--el-table-text-color: var(--el-text-color-primary);
--el-table-row-hover-bg-color: color-mix(in srgb, var(--el-color-primary) 4%, white);
--el-table-row-hover-bg-color: var(--mk-N200);
// --el-table-row-hover-bg-color: var(--el-color-primary-light-9);
--el-table-border-color: var(--el-border-color);
border-radius: var(--radius-xl) var(--radius-xl) 0 0;

Expand All @@ -1165,6 +1169,10 @@
line-height: var(--mk-line-height-base);
padding: calc(var(--spacing) * 3);
}
// 多选列勾选后，整行（含固定列）保持选中底色。
.el-table__row:has(> .el-table-column--selection .el-checkbox.is-checked) > .el-table__cell {
background-color: var(--el-color-primary-light-9);
}
&.small {
.cell {
padding: calc(var(--spacing) * 2) calc(var(--spacing) * 3);

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

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 the review of the provided CSS/SCSS code:

1. Irregularities & Potential Issues

A. Hardcoded Value (32px) in .mk-form-item-with-avatar

  • Issue: calc(32px + var(--spacing) * 3) mixes hardcoded pixel values with design tokens. If the avatar size or spacing token changes, this rule will break visually (misalignment).
  • Risk: Low maintainability. This specific padding likely relies on an external component structure (Element UI + Avatar component). Without a corresponding CSS variable for "avatar width" or "avatar container padding," this is brittle.
  • Suggestion: Extract 32px (and potentially the extra spacing logic) into a CSS variable if possible, e.g., --avatar-offset. If not possible, add a comment explaining why 32px is used (e.g., "Matches default el-avatar size").

B. :has() Selector Support (Critical)

  • Issue: The rule .el-table__row:has(> .el-table-column--selection .el-checkbox.is-checked) uses :has().
  • Compatibility Check: As of current standards, :has() has broad support in modern browsers (Chrome 105+, Safari 15.4+, Firefox 121+). However, if your project targets older browsers or enterprise environments without auto-updates, this rule will be totally ignored by those browsers, causing selection colors to disappear.
  • Suggestion: Verify your browser support requirements. If targeting broader compatibility, this approach is risky without polyfills (which are often heavy or unreliable for complex selectors like this). A JavaScript-free alternative is often difficult; usually, Vue/React state classes are added to the row instead.

C. Specificity & Element Plus Internal Class Names

  • Issue: Reliance on specific internal class names like .el-table-column--selection, .el-table__cell, and .el-form-item__content.
  • Risk: Element Plus (and other UI libraries) can change internal DOM structure or class naming conventions in patch updates. These selectors are fragile.
  • Suggestion: Check if Element Plus provides official props or CSS variables for this behavior. For example, does el-table support highlight-current-row or similar props that achieve this visual effect natively? Native solutions are more stable.

D. Commented-Out Code

  • Issue: // --el-table-row-hover-bg-color: var(--el-color-primary-light-9);
  • Suggestion: Remove this line. Keep commit history clean. If you need this fallback later, rely on source control, not commented-out code in production styles.

2. Optimization Suggestions

A. Use var() for Avatar Offset (Refactor)
If this pattern appears elsewhere, create a variable:

:root {
  --mk-avatar-width: 32px; 
}

.mk-form-item-with-avatar > .el-form-item__content > .el-form-item__error {
  padding-left: calc(var(--mk-avatar-width) + var(--spacing) * 3);
}

B. Alternative to :has() for Row Selection Highlighting
If you must avoid :has(), inspect how Element Plus handles selection. Often, they add a class to the <tr> itself when selected (e.g., .el-table__selected-row). Check the rendered HTML:

  • If there is a native class like .is-selected on the tr: 
    .el-table .el-table__row.el-table__selected-row > .el-table__cell {
   background-color: var(--el-color-primary-light-9);
}
    This is significantly more performant and compatible than :has().

C. Variable Consistency

  • You changed color-mix(...) to var(--mk-N200). Ensure --mk-N200 is a light gray/neutral color.
  • Using var(--el-table-row-hover-bg-color) consistently via variables is good. Just ensure --mk-N200 contrasts enough against text color (WCAG compliance).

Summary of Recommendations

  1. Browser Compatibility: Confirm :has() is acceptable for your target audience. If not, refactor to use JS-applied classes or native Element Plus props.
  2. Fragility: Move away from deep nesting and specific internal EP class names where possible. Look for official customization props.
  3. Hardcoding: Mitigate the 32px hardcode by adding a comment or variable.
  4. Cleanliness: Delete the commented-out row-hover line.

Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/system/chat-management/portal-setting/components/ButtonEditPortal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ function handleClosePortalEdit() {
require-asterisk-position="right"
@submit.prevent
>
<el-form-item label="名称" prop="name">
<el-form-item class="mk-form-item-with-avatar" label="名称" prop="name">
<div class="flex-align-center w-full gap-3">
<MkEditAvatar v-model="portalForm.logo" @change="handleLogoChange" :size="32" :disabled="logoDisabled">
<template #default="{ icon }">
Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/tool/tool-form/DataSourceFormDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ defineExpose({ open })
@submit.prevent
>
<h4 class="mk-title-decoration mb-4">基本信息</h4>
<el-form-item label="名称" prop="name">
<el-form-item class="mk-form-item-with-avatar" label="名称" prop="name">
<div class="flex-align-center w-full gap-3">
<!-- 设置工具头像 -->
<MkEditAvatar v-model="dataSourceForm.icon" @change="handleIconChange">
Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/tool/tool-form/McpFormDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ defineExpose({ open })
@submit.prevent
>
<h4 class="mk-title-decoration mb-4">基本信息</h4>
<el-form-item label="名称" prop="name">
<el-form-item class="mk-form-item-with-avatar" label="名称" prop="name">
<div class="flex-align-center w-full gap-3">
<!-- 设置工具头像 -->
<MkEditAvatar v-model="mcpForm.icon" @change="handleIconChange">
Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/tool/tool-form/SkillToolFormDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ defineExpose({ open })
@submit.prevent
>
<h4 class="mk-title-decoration mb-4">基本信息</h4>
<el-form-item label="名称" prop="name">
<el-form-item class="mk-form-item-with-avatar" label="名称" prop="name">
<div class="flex-align-center w-full gap-3">
<!-- 设置工具头像 -->
<MkEditAvatar v-model="skillForm.icon" @change="handleIconChange">
Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/tool/tool-form/WorkflowFormDialog.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ defineExpose({ open })
require-asterisk-position="right"
@submit.prevent
>
<el-form-item label="名称" prop="name">
<el-form-item class="mk-form-item-with-avatar" label="名称" prop="name">
<div class="flex-align-center w-full gap-3">
<!-- 设置工具头像 -->
<MkEditAvatar v-model="workflowForm.icon" @change="handleIconChange">
Expand Down
9 changes: 7 additions & 2 deletions ui/src/views/tool/tool-form/tool-custom/ToolFormDrawer.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@ defineOptions({ name: 'ToolFormDrawer' })
const { auth } = useStore()

const props = withDefaults(
defineProps<{ api: typeof ToolApi | typeof SystemSharedToolApi | typeof SystemResourceToolApi; title: string; folderId: string; canEdit?: boolean }>(),
defineProps<{
api: typeof ToolApi | typeof SystemSharedToolApi | typeof SystemResourceToolApi
title: string
folderId: string
canEdit?: boolean
}>(),
{ canEdit: true },
)

Expand Down Expand Up @@ -186,7 +191,7 @@ defineExpose({ open })
@submit.prevent
>
<h4 class="mk-title-decoration mb-4">基本信息</h4>
<el-form-item label="名称" prop="name">
<el-form-item class="mk-form-item-with-avatar" label="名称" prop="name">
<div class="flex-align-center w-full gap-3">
<MkEditAvatar v-model="toolForm.icon" @change="handleIconChange">
<template #default="{ icon }">

@shaohuzhang1 shaohuzhang1 Nov 12, 2026

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 the review of the provided code diff:

1. Irregularities & Potential Issues

  • None detected in functionality: The changes are purely stylistic (formatting for readability) and UI consistency (adding a CSS class). They do not alter logic or props definition, so no bugs are introduced.

2. Optimization Suggestions & Best Practices

A. Props Definition Type Safety (TypeScript/Vue)

Using typeof XxxApi directly for the api prop type can be fragile if the API objects have methods that aren't strictly needed here or if their structure changes.

  • Suggestion: Consider defining a minimal interface that includes only the methods the component actually uses. This makes testing easier and decouples the component from specific API implementations. 
    // Example
interface IToolApiClient {
  create: (data: any) => Promise<any>
  update: (id: string, data: any) => Promise<any>
  // ... other used methods
}

props: { api: IToolApiClient }

B. CSS Class Naming Convention

You added mk-form-item-with-avatar.

  • Observation: Hardcoding layout-specific classes (flex-align-center w-full gap-3) inside the template alongside a semantic class (mk-form-item-with-avatar) mixes styling concerns.
  • Suggestion: Ensure mk-form-item-with-avatar handles the avatar positioning internally via its scoped styles or global CSS. If it only provides spacing/alignment, consider using utility classes consistently, or moving all layout logic into the component's own CSS to keep the template clean.

C. Code Formatting (Prettier/ESLint)

  • Good: Breaking down the defineProps generic into multiple lines improves readability significantly, especially as prop count grows. This aligns with common Vue 3 style guides.
  • Check: Ensure your ESLint/Prettier configuration automatically enforces this multi-line format for complex types to prevent future reformatting noise in diffs.

Summary

The changes are safe and beneficial for readability. No functional issues found. Focus on long-term maintainability of the api prop type structure.

Expand Down
Loading