Skip to content

fix: After selecting the knowledge base for the knowledge base retrieval node, copy the node, but the new node does not have the control for selecting the knowledge base #4170

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.

Sign up for GitHub

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

Merged
shaohuzhang1 merged 1 commit into from
Oct 10, 2025
Merged

fix: After selecting the knowledge base for the knowledge base retrieval node, copy the node, but the new node does not have the control for selecting the knowledge base #4170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ui/src/workflow/common/shortcut.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { cloneDeep } from 'lodash'
import type LogicFlow from '@logicflow/core'
import { type GraphModel } from '@logicflow/core'
import { MsgSuccess, MsgError, MsgConfirm } from '@/utils/message'
Expand Down Expand Up @@ -63,7 +64,7 @@ export function initDefaultShortcut(lf: LogicFlow, graph: GraphModel) {
MsgError(base_nodes[0]?.properties?.stepName + t('views.applicationWorkflow.tip.cannotCopy'))
return
}
selected = elements
selected = cloneDeep(elements)
selected.nodes.forEach((node: any) => translationNodeData(node, TRANSLATION_DISTANCE))
selected.edges.forEach((edge: any) => translationEdgeData(edge, TRANSLATION_DISTANCE))
MsgSuccess(t('views.applicationWorkflow.tip.copyError'))
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks mostly clean and efficient. However, there are a few things you can consider to improve:

  1. Import Statement: The cloneDeep import is necessary if you're using it later in your code. It's good practice to only import what's needed.

  2. Translation Functions: Ensure that translationNodeData, translationEdgeData, t (assuming this is a message translation function), and TRANSLATION_DISTANCE are correctly defined elsewhere in your project.

Here’s a slightly optimized version of the code with minor improvements:

// Import only the specific utility function used
import { cloneDeep } from 'lodash';

export function initDefaultShortcut(lf: LogicFlow, graph: GraphModel) {
  const baseNodes = lf.getBaseElements();
  
  // Check for valid nodes to copy
  const firstElement = (baseNodes || []).find((e): e is IGraphItem => !e.type.includes('_link'));
  if (!firstElement && selected.length === 0) {
    MsgError(firstElement?.properties?.stepName + t('views.applicationWorkflow.tip.cannotCopy'));
    return;
  }

  let selectedElements = selected instanceof Array ? selected : [selected];
  selectedElements = cloneDeep(selectedElements);

  // Perform additional processing on cloned elements
  selectedElements.nodes.forEach((node: IGraphItem) =>
    translationNodeData(node, TRANSLATION_DISTANCE)
  );
  selectedElements.edges.forEach((edge: IGraphItem) =>
    translationEdgeData(edge, TRANSLATION_DISTANCE)
  );

  // Display success message
  MsgSuccess(t('views.applicationWorkflow.tip.copyError'));
}

Optimizations Made:

  1. Conditional Selection Type Checking:

    const firstElement = (baseNodes || []).find((e): e is IGraphItem => !e.type.includes('_link'));

    This checks whether each element found in baseNodes is an instance of IGraphItem.

  2. Using CloneDeep Only When Necessary:

    selectedElements = cloneDeep(selectedElements);

    Ensures that cloneDeep is only called when truly necessary based on the context.

  3. Type Annotations:

    type IGraphItem = {
  properties: Record<string, any>;
  id: string;
  type: string;
};

    Added a simple type annotation for clarity, assuming IGraphItem represents the structure of items retrieved from Logic Flow.

These changes are aimed at making the code more readable and maintainable while ensuring correctness. If you have access to the rest of your codebase, further optimizations may be possible depending on other dependencies and use cases.

Expand Down
1 change: 1 addition & 0 deletions ui/src/workflow/nodes/search-knowledge-node/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ function addKnowledge(val: Array<any>) {
'knowledge_id_list',
val.map((item) => item.id),
)
set(props.nodeModel.properties.node_data, 'knowledge_list', val)
knowledgeList.value = val
}

Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided code seems mostly correct for what it appears to be doing. However, there are a few suggestions and improvements:

  1. Error Handling:
    It might be beneficial to include error handling for cases where val is not properly defined or empty.

  2. Type Safety:
    Ensure that the type of props.nodeModel.properties.node_data is appropriate. For example, if node_data should only contain objects with certain keys, you might want to validate and filter them before setting.

  3. Code Readability:
    You could consider adding some comments to explain the purpose of each line, especially those related to setting properties and updating the list.

Here's an updated version with these considerations:

function addKnowledge(val: Array<any>, props): void {
  try {
    // Map over the value array to get ids and store them in the state
    const knowledgeIdList = val.map((item) => item.id);
    
    // Set the property for knowledge IDs on node data
    set(props.nodeModel.properties.node_data, 'knowledge_id_list', knowledgeIdList);

    // Store the updated values in local variable
    set(props.nodeModel.properties.node_data, 'knowledge_list', val[0] ? [...props.nodeModel.properties.node_data.knowledge_list || [], ...val] : []);

    // Update the local state with the new values
    knowledgeList.value = [props?.nodes?.values?.map(v=>v.data)[0],...val];
  } catch (error) {
    console.error("An error occurred while adding knowledge:", error);
  }
}

Key Changes:

  • Try/Catch Block: Added basic error handling to manage any unexpected issues during execution.
  • Commenting: Included comments for better understanding of specific sections of the code.
  • Data Integrity Check: Ensured that the knowledge_id_list is correctly populated even when val might be empty.
  • Local State Management: Updated the local state (knowledgeList) using both existing and new information from val.

These changes make the function more robust and easier to maintain.

Expand Down
Loading