Skip to content

fix: Whether to display execution details depends on the configuration. - #7820

Merged
shaohuzhang1 merged 1 commit into
v3from
pr@v3@fix_show_exec
Nov 10, 2026
Merged

shaohuzhang1 merged 1 commit into
v3from
pr@v3@fix_show_exec

Conversation

@shaohuzhang1

Copy link
Copy Markdown
Contributor

fix: Whether to display execution details depends on the configuration.

<el-button v-if="show_exec" text @click="openExecutionDetail" class="-ml-1">
<MkIcon name="icon_logs_outlined" />
<span>执行详情</span>
</el-button>

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 a review of the code changes with identified issues and optimization suggestions:

1. Code Style & Formatting

  • Missing Spaces: const show_exec=computed(...) lacks spacing around the equals sign and arrow function. It should be const showExec = computed(() => ...).
  • Naming Convention: show_exec uses snake_case. Vue/JS conventions typically prefer camelCase for variables (showExec). Additionally, exec is an ambiguous abbreviation; executionDetailsVisible or canViewExecution would be clearer.
  • Import Usage: You imported useMessageListStore but do not seem to use it in the diff. If unused, remove it to prevent lint errors and confusion.

2. Logic & Safety Issues

  • Optional Chaining in Computed:
    const show_exec = computed(() => messageList?.appInfo.value?.show_exec)
    While optional chaining prevents crashes, v-if="show_exec" will evaluate as false if messageList is null or undefined. This is safe, but ensure that the intent is to hide the button when data isn't ready, rather than hiding it due to a missing provider.
  • Reactivty Check: Ensure messageList?.appInfo is truly reactive. If appInfo is assigned once and doesn't change, accessing .value inside the computed property getter might cause unnecessary dependency tracking if not handled carefully. However, since messageList is injected, this is likely correct provided useMessageListStore returns reactive refs.

3. Optimization Suggestions

  • Hoisting Logic: The computed property show_exec is defined after canShare. Group related states together for better readability.
  • Template Clarity: In the template, v-if="show_exec" relies entirely on the state of messageList. If useShowShare() or other external factors affect visibility, consider combining them:
    const showExecutionButton = computed(() => 
      Boolean(messageList?.appInfo?.value?.show_exec)
    );
    (Note: Removed redundant .length check if show_exec is already a boolean flag).

Recommended Refactored Code Snippet

import { MESSAGE_LIST_KEY } from '../message-list/index'; // Removed unused useMessageListStore
// ... other imports

// 智能体允许分享时显示分享入口;进入分享时预先勾选当前回答所属轮次。
const showShare = useShowShare()
const shareLink = useShareLinkStoreOptional()

const messageList = inject(MESSAGE_LIST_KEY, null)
const detail = useExecutionDetailStore()

// 回答生成中禁用分享。
const canShare = computed(() => Boolean(messageList?.messages?.value?.length && !messageList?.streamLoading?.value))

// ✅ Fixed naming, spacing, and logic clarity
const showExec = computed(() => {
  // Optional chaining ensures safe access even if messageList or appInfo is null
  return Boolean(messageList?.appInfo?.value?.show_exec);
})

// ...

<el-button 
  v-if="showExec" 
  text 
  @click="openExecutionDetail" 
  class="-ml-1"
>
  <MkIcon name="icon_logs_outlined" />
  <span>执行详情</span>
</el-button>

Summary of Actions:

  1. Remove unused import useMessageListStore.
  2. Rename show_exec to showExec (camelCase).
  3. Fix Format: Add spaces around = and arrows.
  4. Keep v-if but ensure the variable name clearly indicates its boolean nature.

@shaohuzhang1
shaohuzhang1 merged commit b613978 into v3 Nov 10, 2026
3 of 4 checks passed
@shaohuzhang1
shaohuzhang1 deleted the pr@v3@fix_show_exec branch November 10, 2026 10:57
v-if="(!isShared && msg.role === 'ASSISTANT' && (msg.write_ed || msg.record))&&!streamLoading"
:message="msg"
@resize="scrollRef?.followBottom()"
/>

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 snippet provided:

1. Irregularities & Issues

  • Formatting Spacing: There is a missing space between ) and ! in (msg.write_ed || msg.record))&&!streamLoading. While this is valid syntax, it violates standard formatting guidelines (e.g., Prettier, ESLint) and reduces readability.
    • Fix: Add a space: ...)) && !streamLoading.
  • Boolean Logic Order: In Vue templates, v-if conditions are evaluated every render cycle. The condition isShared && msg.role === 'ASSISTANT' ... involves multiple checks. If isShared is true, the rest of the expression short-circuits correctly. However, mixing template display logic (!isShared) with component state (!streamLoading) directly in the template can make the view harder to maintain.
  • Potential Race Condition/Stuttering: If streamLoading toggles frequently during streaming, this condition will cause the <MessageFooter> component to mount/unmount repeatedly. This leads to:
    • Unnecessary DOM re-renders.
    • Flickering if the footer has animations or transitions.
    • Performance overhead due to creating/destroying the component instance.

2. Optimization Suggestions

Option A: Extract to a Computed Property (Recommended)

Move the complex boolean logic into a computed property for better performance, testability, and readability.

// Script setup
const shouldShowFooter = computed(() => {
  if (isShared.value) return false;
  if (streamLoading.value) return false;
  if (msg.role !== 'ASSISTANT') return false; // Assuming 'msg' is available in scope
  
  return !!msg.write_ed || !!msg.record; // Ensure explicit boolean check if needed
});
<!-- Template -->
<MessageFooter
  v-if="shouldShowFooter"
  class="mt-3"
  :message="msg"
  @resize="scrollRef?.followBottom()"
/>

Option B: Use v-show Instead of v-if (If Footer is Heavy)

If <MessageFooter> is expensive to initialize and you only want to toggle visibility without unmounting, use v-show. However, note that v-show keeps the component mounted even when hidden.

<MessageFooter
  v-show="(!isShared && msg.role === 'ASSISTANT' && (msg.write_ed || msg.record)) && !streamLoading"
  ...
/>

Note: Only use v-show if the footer contains heavy initial computations or internal state that should persist. For simple UI elements, v-if is generally preferred.

3. Corrected Code Snippet (Quick Fix)

If you prefer to keep the inline logic, at least fix the formatting:

<MessageFooter
  class="mt-3"
  v-if="(!isShared && msg.role === 'ASSISTANT' && (msg.write_ed || msg.record)) && !streamLoading"
  :message="msg"
  @resize="scrollRef?.followBottom()"
/>

Summary

  • Critical Issue: None (code works functionally).
  • Best Practice Violation: Formatting ()&&! → ) && !).
  • Architectural Improvement: Extract boolean logic to a computed property to separate presentation logic from template markup and improve render performance stability.

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.

1 participant