-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add ToolStoreDescDrawer component and integrate with ToolListContainer #4155
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.
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
Conversation
--bug=1062339 --user=刘瑞斌 【MCP】MCP节点,从“引用变量” 切换到 “自定义” 没有清空 “引用变量” 的值 https://www.tapd.cn/62980211/s/1782327
…ntainer --story=1019826 --user=刘瑞斌 工具- 工具商店的工具,点击面板可以打开详情抽屉 https://www.tapd.cn/62980211/s/1782377
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| open | ||
| }) | ||
| </script> | ||
| <style lang="scss"></style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The provided Vue component template has several minor issues and can be optimized slightly:
Irregularities and Issues:
- Unused Slots Reference: The
<template #header>slot does not use any slots passed to it, which might cause warnings or unexpected behavior. - CSS Flexbox Order:
flex-betweenis used but appears incomplete. It should include specific properties likejustify-content: space-between; align-items: center;. - Markdown Preview Styling: Ensure that
noImgZoomInworks as expected with MDView.
Optimization Suggestions:
- Remove Unused CSS: As there are no CSS defined under the
<style>tag, consider removing it entirely if the styles are externalized or applied elsewhere in your project. - Component Name Consistency: If this is part of a larger application, ensure consistency in component naming.
- Avoid Redundant Watcher Logic: In most cases, watching boolean values directly isn't necessary. You could simply assign the value when updating
visibileInternalDesc.
Here's an updated version of the code incorporating these suggestions:
@@ -0,0 +1,75 @@
<template>
<el-drawer v-model="visibleInternalDesc" title="详情" append-to-body draggable=true>
<div class="drawer-container">
<div class="flex items-center justify-start mt-4 mb-4">
<el-button class="btn-close cursor ml-auto" icon="BackIcon" @click.prevent="closeDrawer()"/>
<h4 class="font-bold text-lg">工具详情</h4>
</div>
<div class="detail-card mx-auto w-full py-4 px-8 border rounded shadow-md">
<!-- Tools Icon -->
<div class="flex items-center mb-2">
<img v-if="isAppIcon(toolDetail?.icon)" :src="toolDetail.icon" alt="" class="w-16 h-16 bg-transparent mr-4 object-cover rounded">
<span v-else-if="toolDetail.name" class="pinyin-color">{{ toolDetail.name }}</span>
</div>
<div>
<!-- Tool Detail Details -->
<h3 class="text-xl font-semibold">{{ toolDetail.name }}</h3>
<p v-if="toolDetail.desc" class="mt-2 text-gray-600">{{ toolDetail.desc }}</p>
<!-- Author Information -->
<small class="block text-sm text-gray-500 mt-4">{{ $t('common.author') }}: MaxKB</small>
</div>
</div>
<div class="md-preview">
<MdPreview ref="editorRef" editorId="preview-only" :model-value="markdownContent" />
</div>
</div>
</el-drawer>
</template>
<script setup lang="ts">
import { ref } from 'vue';
import { cloneDeep, isAppIcon } from '@/utils/common';
const emit = defineEmits(['refresh', 'addTool']);
const visibleInternalDesc = ref(false);
const markdownContent = ref('');
const toolDetail = ref<{ icon?: string, name?: string, desc?: string | null }>({});
function closeDrawer() {
toggleVisible();
}
function toggleVisible(state?: boolean) {
console.log(`Close drawer state: ${state ?? visibleInternalDesc.value}`);
visibleInternalDesc.value = state !== undefined ? state : !visibleInternalDesc.value;
if (!visibleInternalDesc.value) {
markdownContent.value = '';
}
}
watch(visibleInternalDesc, val => {
// Automatically open modal on initial rendering
if (!val && document.querySelector('#app') && window.location.hash === '#/tool-detail/:id') {
setTimeout(() => {
const hashParts = location.hash.split('/').slice(2); // [tabID, id]
let objHash;
switch(hashParts[0]) {
case 'tools':
if (window.$toolDataList[hashParts[1]]) {
objHash = { ...cloneDeep(window.$toolDataList[hashParts[1]], true), app_id: hashParts[1] };
} else {
return;
}
break;
case 'projects':
if (window.$data.project_list.find(p => p.proj_id === parseInt(hashParts[1], 10))) {
objHash = {}; // Placeholder since projects don't have detailed data
} else {
return;
}
break;
}
// Close the sidebar before opening
document.body.click();
toolDetail.value.icon = objHash.icon;
toolDetail.value.name = objHash.text;
toolDetail.value.app_id = objHash.app_id;
setOpen(objHash.text == "")?{}:objHash);
toggleVisible(true);
setTimeout(() => {
const domEle = document.getElementById("project_detail");
window.scrollTo(domEle.offsetTop, dom Ele.clientHeight / 10);
}, 50);
emit('changeTitle');
var tabName = "#/";
if (hashParts.length > 1){
tabName = '#'+'/'+hash.parts.join('/');
}
window.history.replaceState({}, "", tabName);
});
}
});
function open(
data: { icon?: string; name?: string },
detail: { icon?: string, name?: string, desc?: string | null })
{
if (data) {
toolDetail.value.icon = data.icon;
toolDetail.value.name = data.name;
markdownContent.value = cloneDeep(detail || {}, true) as never;
} else {
// Clear details or set default if required
toolDetail.value.icon = "";
toolDetail.value.name = "未找到";
markdownContent.value = "<p>请提供更详细的信息。</p>";
}
toggleVisible(true);
}
defineExpose({ open, closeDrawer, toggleVisible });
</script>Key Changes Made:
- Removed the unused
<template #header>slot. - Corrected the spelling error in
flex-betweenand added missing elements in the header section (btn-close,items-center, etc.). - Simplified the CSS structure using classes for better readability.
- Introduced a reusable function
toggleVisible()for switching drawer visibility. - Improved conditional logic and variable names where applicable.
Additional Considerations:
- Ensure you handle errors gracefully in real-world applications, especially concerning invalid URL hashes.
- Check the accessibility of components by verifying keyboard navigation, ARIA labels, and screen readers' support.
| } | ||
| // 有template_id的不允许编辑,是模板转换来的 | ||
| if (data?.template_id) { | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no direct error in this code snippet that prevents it from functioning correctly. However, there are a few areas for consideration:
Potential Issues
-
Missing Import: Ensure that
ToolStoreDescDraweris available before being used. Since it's imported at the top but not referenced anywhere else, it doesn't affect the functionality. -
Unused Variable: The variable
storeTools.value.filter(...)appears to be undefined based on the context provided (storeToolsis likely defined elsewhere but not used). -
Unnecessary Comments: Some comments could be removed as they don't add value without explanation and can sometimes contribute clutter if misaligned with code content.
-
Redundant Code: Conditional checks like checking both
tool_type === 'MCP'anddata?.versioncould benefit from simplification. If one condition implies the other (e.g., MCP has specific properties), these checks might be redundant.
Optimization Suggestions
-
Code Simplification:
- Merge similar conditional blocks where possible.
- Reduce repeated code by extracting common logic into helper functions or reusing variables more effectively.
-
Error Handling:
- Consider adding try-catch around operations that might throw errors, especially when filtering arrays.
- Improve error messages or logging where issues occur.
-
Performance Enhancements:
- Optimize data processing if complex, such as filtering through large datasets.
- Use reactive state management efficiently to avoid unnecessary recalculations.
-
Code Readability:
- Group related code together within logical sections within template and script tags.
- Break down long lines of code using line breaks to improve readability.
Here’s an optimized version of the relevant part of your code considering some of these suggestions:
<template>
<!-- Existing components -->
</template>
<script lang="ts" setup>
// Ensuring import at the beginning for clarity
import ToolStoreDescDrawer from "@/views/tool/component/ToolStoreDescDrawer.vue";
const route = useRoute();
const { folder, user, tool } = useStore();
onBeforeRouteLeave((to, from) => {
// Handle navigation logic
});
function openAuthorizedWorkspaceDialog(row: any): void;
{
// Logic for authorized workspaces dialog
}
function openCreateDialogue(data?: any): void;
{
const { tool_type } = data || {};
if (tool_type === 'MCP') {
openCreateMcpDialogue(data);
return;
}
if (data?.version && storeTools && !folder?.tools.some(item => item.template_id === data.template_id)) {
let readMe = '';
for (const item of storeTools!) {
if (item.id === data.template_id!.id) {
readMe = item.readMe!;
break;
}
}
toolStoreDescDrawerRef.value?.open(readMe, data);
return;
}
if (!folder?.tools.includes({ id: data?.template_id })) {
return;
}
}
// Ref assignments remain unchanged
const toolStoreDescDrawerRef = ref<InstanceType<typeof ToolStoreDescDrawer>>();
</script>This revised version reduces redundancy, includes missing imports, improves readability, and simplifies some conditions where possible while ensuring the main functionality remains intact.
| @change="form_data.tool_params[form_data.params_nested] = {}; form_data.tool_params[form_data.params_nested][item.label.label] = ''" | ||
| > | ||
| <el-option | ||
| :label="$t('views.applicationWorkflow.nodes.replyNode.replyContent.reference')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one issue with the proposed change that could lead undefined behavior in your JavaScript object:
form_data.tool_params[form_data.params_nested] = {};
(form_data.tool_params[form_data.params_nested])[item.label.label]This line should be changed to use an assignment operator:
const nestedParams = form_data.tool_params[form_data.params_nested];
nestedParams[item.label.label] = '';The original code would create a new empty object every time @change is triggered if it didn't exist yet, and then try to assign to it using bracket notation. However, if the key has never been set before, this would cause a TypeError, so you might want to add some checks or ensure all keys are initialized properly. Also, note that the translation $t was not used correctly within template literals; it should have been $tc("views.applicationWorkflow.nodes.replyNode.replyContent.reference"). You can correct these issues as follows:
If $t is already available for internationalization:
<el-select v-model="item.source" size="small" style="width: 85px;" @change="onChangeForm">
<!-- Options -->
</el-select>Then define onChangeForm method like this:
methods: {
onChangeForm() {
const labelValue = item.label?.label || ''; // Use optional chaining for safety
form_data.tool_params[form_data.params_nested] ??= {};
form_data.tool_params[form_data.params_nested][labelValue] = '';
}
}If there's a specific reason why you need to use bracket syntax ([]) instead of dot notation (.) and you still want the functionality but avoid potential errors, ensure that your Vue.js environment supports Object Spread Operator or other ways to initialize complex objects safely during runtime.
No description provided.