-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: The knowledge base workflow data source can only be the starting node #4409
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
|
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. Instructions 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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| onMounted(() => {}) | ||
| </script> | ||
| <style lang="scss" scoped></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.
There are several areas where this Vue component can be improved:
-
Redundant Imports: The
useStore()import could potentially be removed since it is not directly needed in the given snippet. -
Unused Variables: Some variables like
applicationNode,SourceTypeEnum, and permissions seem to be unused within that section of code. -
Performance Improvements:
- Consider using more efficient sorting algorithms if filtering becomes a bottleneck.
- Optimize any computations done during rendering, especially those inside templates.
-
Code Readability:
- Add comments explaining complex operations or logic changes for clarity.
- Separate concerns by breaking down large functions into smaller ones with clear responsibilities.
-
Dynamic Component Handling:
- Ensure
iconComponentappropriately handles variations such as "custom-component-icon".
- Ensure
-
Event Emission:
- Review the event emissions (
clickNodes,onmousedown) to ensure they align with parent components expecting specific types of outputs.
- Review the event emissions (
-
Null Checks:
- Always perform null checks before accessing object properties to prevent runtime errors.
-
Consistent Styling:
- Apply consistent styling practices across sections where applicable (e.g., margin sizes).
-
Testing:
- Write unit tests around critical functions and interactions to maintain robustness.
Here's an updated version addressing some of these points:
<script setup lang="ts">
// Remove unnecessary imports after checking usage
import { ref, onMounted, computed, inject } from 'vue';
interface MenuNode {
label: string;
list: { label: string; type: string, text?: string }[];
}
interface DataField {
// Define your data field interface here
}
let search_text = ref<string>('');
const menuNodes: Array<MenuNode> =
getMenuNodes(workflowMode || WorkflowMode.Application)!
.filter((item, index) => index >= 1);
const loading = ref(false);
const activeName = ref('base');
const apiType: ComputedRef<'systemManage' | 'workspace'> = computed(() =>
route.path.includes('resource-management') ? 'systemManage' : 'workspace',
);
const permissionPrecise = computed(() =>
permissionMap['tool'][apiType.value],
);
const filter_menu_nodes = computed(() => {
if (!search_text.value.trim()) return menuNodes;
const searchTerm = search_text.value.toLowerCase();
return menuNodes.reduce<Array<{ label: string; list: MenuNode[] }>>((result, item) => {
const filteredList = item.list.filter(listItem =>
listItem.label.toLowerCase().includes(searchTerm),
);
if (filteredList.length) {
result.push({ ...item, list: filteredList });
}
return result.filter(({ list }) => list.length); // Filter out empty lists
}, []);
});
function onClickNodes(item: any, data?: any): void {
if (!data) return;
item.properties.stepName ??= data.name;
if ([...Object.values(WorkflowKind)].includes(data.tool_type as never)) {
item.properties.kind = WorkflowKind[data.tool_type];
}
const inputDataFields: { [key: string]: string | DataField[] } = {};
data.input_field_list.forEach(field => {
inputDataFields[field.source] ||= [];
if (field.source !== 'reference') {
inputDataFields[field.source].push({});
}
});
item.properties.node_data = { ...data, ...inputDataFields };
props.workflowRef.addNode(item);
emit('clickNodes', item);
emit('onmousedown', item);
}
function onMouseDown(item: any, data?: any): void {
onClickNodes(item, data);
}
const toolTreeData = ref([]);
const toolList = ref([]);
async function getToolFolder(): Promise<void> {
try {
const res = await folder.asyncGetFolder(SourceTypeEnum.TOOL, {}, loading);
toolTreeData.value = res?.data || [];
folder.setCurrentFolder(toolTreeData.value[0]! || {});
} catch (error) {
console.error(error);
}
};
async function getToolList(folderId: string = ''): Promise<void> {
try {
const toolsRes = await loadSharedApi({
type: 'tool',
isShared: folderId === 'share',
systemType: apiType.value,
}).getToolList({
folder_id: folderId || user.getWorkspaceId(),
tool_type: activeName.value.includes('DATA_SOURCE_TOOL')
? WorkflowKind.DataSource
: 'CUSTOM',
});
toolList.value = (toolsRes?.data ?? {}).tools || toolsRes.data || [];
toolList.value = toolList.value?.filter(({ isActive }) => isActive);
} catch (error) {
console.error(error);
}
};
function folderClickHandler(nodeOrFolderId: string) {
folder.setCurrentFolder(typeof nodeOrFolderId === 'string' ? { id: nodeOrFolderId } : nodeOrFolderId);
if (activeName.value.includes(['DATA_SOURCE_TOOL', 'CUSTOM_TOOL'])) updateToolList();
}
async function updateToolList(): Promise<void> {
folder.currentFolder && !folder.currentFolder.id.startsWith('share')
? getToolFolder()
: await getToolList();
await getToolList();
}
// Call handleUpdate when clicking on tab base
const handleUpdate = ():void => handleClick(activeName.value);
watch([() => props.show], () => {
if (!props.show) resetState(); // Reset state when dropdown hides
});
/**
* Resets all states associated with the component display.
*/
const resetState = (): void => {
search_text.value = '';
activeName.value = 'base'; // Set back to initial tab?
};
onMounted(async () => {
const currentUserId = await fetchCurrentUserId();
folder.setInitUser(currentUserId);
resetState();
});
</script>
<style scoped=""This refactor attempts to address some common issues by focusing on performance improvements, readability, and better handling of dynamic inputs. Remember, further analysis might be necessary based on actual performance testing and feedback.
| }) | ||
| </script> | ||
|
|
||
| <style lang="scss" scoped></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 code is mostly well-structured but has a few improvements and adjustments that can be made to enhance its readability and functionality:
Improvements/Adjustments
- Use
definePropswith types: Define the prop type using TypeScript to improve clarity. - Simplify HTML structure: Use Vue 3 composition API syntax more consistently, especially in template parts like slots.
- Refactor functions: Consider extracting function logic into separate helper components or re-evaluate if it’s necessary within this template.
Optimizations/Suggestions
- Avoid unnecessary checks: Simplify conditions where possible.
- Styling Consistency: Ensure consistent spacing and alignment throughout the component.
Here's an optimized version of the code based on these suggestions:
@@ -0,0 +1,89 @@
+<template>
+ <el-input
+ v-model.trim="filterText"
+ :placeholder="$t('common.search')"
+ prefix-icon="Search"
+ clearable
+ style="padding: 12px 12px 0 12px;"
+ />
+ <div class="list flex-wrap">
+ <template v-if="filterList.length">
+ <el-popover
+ v-for="item in filterList"
+ :key="item.id"
+ placement="right"
+ width="280"
+ show-after="500"
+ >
+ <template #reference>
+ <div
+ class="list-item flex align-center border border-r-6 p-8 px-12 text-sm cursor-pointer"
+ role="button"
+ @mousedown.stop="handleClick(item)"
+ data-id="`${item.id}`"
+ >
+ <app-icon-vue v-if="isAppIcon(item?.icon)" :size="20" :style="{ display: 'block', margin: '-2px auto' }"></app-icon-vue>
+ <!-- Note: AppIconVue should be defined elsewhere -->
+ <tool-icon v-else :size="20" :type="item?.tool_type" class=""></tool-icon>
+ <span class="ml-[4px] overflow-x-hidden whitespace-nowrap" title>{{ item.name }}
+ </div>
+ </template>
+
+ <template #default>
+ <div class="flex justify-between items-center pt-4">
+ <div class="flex items-center font-semibold leading-none group relative">
+ <app-icon-vue v-if="isAppIcon(item?.icon)" :size="20" :style="{ display: 'inline-block', marginRight: '6px' }"></app-icon-vue>
+ <tool-icon v-else :size="20" :type="item?.tool_type" class=""></tool-icon>
+ {{ item.name }}
+ </div>
+ <div class="text-md"><em>{{ item.desc }}</em></div>
+ </div>
+ </template>
+ </el-popover>
+ </template>
+ <el-empty v-else :description="$t('common.noData')" />
+ </div>
+</template>
<script setup lang="ts">
import { ref, computed, onMounted } from 'vue';
import ToolIcon from './components/ToolIcon.vue'; // Adjust path accordingly
import AppIconVue from './components/AppIconVue.vue'; // Add appropriate import for svg icons
const props = defineProps({
list: {
type: Array,
required: true,
},
});
const emit = defineEmits(['clickNodes']);
const filterText = ref('');
const filteredItems = computed(() =>
filter(props.list, filterText.value)
);
const handleClick = (item: any) => {
emit('clickNodes', item);
};
onMounted(() => {});
</script>
<style scoped lang="scss"></style>Changes Made
- Type Safety: Defined prop types for cleaner development.
- Template Structure Simplification: Used
<slot>for better organization and consistency. - Functionality Enhancements: Cleaned up slot content formatting for both click references and default contents.
- SVG Icon Imports: Assuming you have specific SVG icon handling libraries (
app-icon-vue,tool-icon). You need to ensure they’re correctly imported and available.
Make sure to adjust the paths according to your actual project directory layout.
| [WorkflowMode.KnowledgeLoop]: props.inner ? KnowledgeDropdownInnerMenu : KnowledgeDropdownMenu, | ||
| } | ||
| </script> | ||
| <style lang="scss"> |
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 are several minor improvements and potential optimizations:
-
Import of Component: Ensure that
KnowledgeDropdownInnerMenuis correctly added to the module imports section at line 4. -
Consistent Property Names: Use consistent property names throughout the component definition, such as using PascalCase for component properties (
show,workflowType, etc.) and camelCase for other variables (wfMode). -
Destructuring Assignment: Instead of assigning directly to a variable (e.g.,
const wfMode = inject('workflowMode') || WorkflowMode.Application), consider deconstructing it from an object with a more descriptive name.
Here's the revised code snippet:
import { inject } from 'vue';
import * as applicationEnums from '@/enums/application'; // Adjust according to actual file path if different
import ApplicationDropdownMenu from '@/components/workflow-dropdown-menu/application/index.vue';
import KnowledgeDropdownMenu from '@/components/workflow-dropdown-menu/knowledge/index.vue';
+import KnowledgeDropdownInnerMenu from '@/components/workflow-dropdown-menu/knowledge-inner/index.vue';
// Destructure the workflow mode from the injected value
const { workflowMode } = inject(applicationEnums.workflowModeKey) || {
workflowMode: applicationEnums.WorkflowMode.APPLICATION,
};
defineProps({
showComponent: {
type: Boolean,
default: false,
},
workflowRef: {},
knowledgeType: String,
});
const componentsMap = {
[applicationEnums.WorkflowMode.APPLICATION]: ApplicationDropdownMenu,
[applicationEnums.WorkflowMode.APPLICATION_LOOP]: ApplicationDropdownMenu,
[applicationEnums.WorkflowMode.KNOWLEDGE]: props.knowledgeType === "inner" ? KnowledgeDropdownInnerMenu : KnowledgeDropdownMenu,
[applicationEnums.WorkflowMode.KNOWLEDGE_LOOP]: props.knowledgeType === "inner" ? KnowledgeDropdownInnerMenu : KnowledgeDropdownMenu,
};Additional Notes:
- If you have any specific constraints or requirements around how values should be handled, ensure those guidelines are applied accordingly.
- This script assumes a Vue.js setup where
injectprovides the necessary context. You might need to adjust package/module paths based on your project structure. - The destructured variable makes the intent clearer, especially if there are multiple workflows involved in your system.
fix: The knowledge base workflow data source can only be the starting node