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

[shaohuzhang1]

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

[f2c-ci-robot]

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.

Details

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.

[f2c-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaohuzhang1]

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.

[shaohuzhang1]

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.