Repository navigation
fix: Whether to display execution details depends on the configuration. - #7820
Merged
Merged
Conversation
shaohuzhang1
commented
Nov 10, 2026
| <el-button v-if="show_exec" text @click="openExecutionDetail" class="-ml-1"> | ||
| <MkIcon name="icon_logs_outlined" /> | ||
| <span>执行详情</span> | ||
| </el-button> |
Contributor
Author
There was a problem hiding this comment.
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 beconst showExec = computed(() => ...). - Naming Convention:
show_execuses snake_case. Vue/JS conventions typically prefer camelCase for variables (showExec). Additionally,execis an ambiguous abbreviation;executionDetailsVisibleorcanViewExecutionwould be clearer. - Import Usage: You imported
useMessageListStorebut 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:
While optional chaining prevents crashes,
const show_exec = computed(() => messageList?.appInfo.value?.show_exec)
v-if="show_exec"will evaluate asfalseifmessageListis 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?.appInfois truly reactive. IfappInfois assigned once and doesn't change, accessing.valueinside the computed property getter might cause unnecessary dependency tracking if not handled carefully. However, sincemessageListis injected, this is likely correct provideduseMessageListStorereturns reactive refs.
3. Optimization Suggestions
- Hoisting Logic: The
computedpropertyshow_execis defined aftercanShare. Group related states together for better readability. - Template Clarity: In the template,
v-if="show_exec"relies entirely on the state ofmessageList. IfuseShowShare()or other external factors affect visibility, consider combining them:(Note: Removed redundantconst showExecutionButton = computed(() => Boolean(messageList?.appInfo?.value?.show_exec) );
.lengthcheck ifshow_execis 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:
- Remove unused import
useMessageListStore. - Rename
show_exectoshowExec(camelCase). - Fix Format: Add spaces around
=and arrows. - Keep
v-ifbut ensure the variable name clearly indicates its boolean nature.
shaohuzhang1
commented
Nov 10, 2026
| v-if="(!isShared && msg.role === 'ASSISTANT' && (msg.write_ed || msg.record))&&!streamLoading" | ||
| :message="msg" | ||
| @resize="scrollRef?.followBottom()" | ||
| /> |
Contributor
Author
There was a problem hiding this comment.
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.
- Fix: Add a space:
- Boolean Logic Order: In Vue templates,
v-ifconditions are evaluated every render cycle. The conditionisShared && msg.role === 'ASSISTANT' ...involves multiple checks. IfisSharedis 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
streamLoadingtoggles 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
computedproperty to separate presentation logic from template markup and improve render performance stability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: Whether to display execution details depends on the configuration.