[shaohuzhang1]

perf: style

[shaohuzhang1]

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.

[shaohuzhang1]

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.

[shaohuzhang1]

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.