fix: Node copied in orchestration, default name incorrect, should be in numerical calculation form of+1, not string form of+1

[shaohuzhang1]

fix: Node copied in orchestration, default name incorrect, should be in numerical calculation form of+1, not string form of+1

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

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.

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]

Code Review and Suggestions:

// Import statements
import { nodeDict } from '@/workflow/common/data';
import { isActive, connect, disconnect } from './teleport';
import { t } from '@/locales';
import { type Dict } from '@/api/type/common';

// Function to generate unique step names
const getNodeName = (nodes: Array<any>, baseName: string): string => {
  let index = 0;
  let newName = baseName;

  while (true) {
    if (newName.trim() in nodes.map(node => node.properties.stepName)) {
      index++;
      newName = `${baseName}${index}`;
    } else {
      return newName;
    }
  }
};

class AppNode extends HtmlResize.view {
  // Existing fields and methods

  // Update method to handle step name uniqueness
  update(props: any) {
    super.update(props);

    if (props.model.properties.noRender) {
      delete props.model.properties.noRender;
    } else {
      const filterNodes = props.graphModel.nodes.filter((node: any) => node.type === props.model.type);
      
      // Generate a unique step name if it already exists
      const currentStepName = props.model.properties.stepName || '';
      if (filterNodes.some(node => node.properties.stepName.toLowerCase() === currentStepName.trim().toLowerCase())) {
        const uniqueStepName = getNodeName(
          props.graphModel.nodes.filter(n => n.id !== props.model.id),
          currentStepName
        );
        if (uniqueStepName) {
          props.model.properties.stepName = uniqueStepName;
        }
      }

      props.model.properties.config = nodeDict[props.model.type].properties.config;
      if (props.model.properties.height && !isNaN(Number(props.model.properties.height))) {
        props.model.height = Number.parseInt(props.model.properties.height, 10);
      }
    }
  }
}

export default AppNode;

Potential Improvements:

1. Avoid Using console.log: While console.log can be helpful during development, it should be used judiciously and removed before production. Ensure that logs are not present in final output.

2. **Use .includes Instead of map.filter: The use of map(nodes).filter(...)creates an intermediate array when filtering nodes with property matches. Use.includes()directly on the array returned byfilter(...)` which is more efficient and readable.

3. Consistency in Variable Names: Use consistent naming conventions throughout the codebase and avoid inconsistent case usage within the same variable.

4. Edge Case Handling: Consider adding additional checks for edge cases, such as empty arrays or properties like null values affecting the step name generation logic.

5. Optimize Regular Expression Usage: If necessary, consider using regular expressions sparingly to improve performance, especially when dealing with large datasets. In this context, it seems unnecessary given the approach taken.

These improvements aim to make the code cleaner, more performant, and easier to maintain.

[shaohuzhang1]

The current implementation is mostly correct but can be optimized and improved slightly with a few suggestions:

Optimizations/Suggestions

1. Avoid Multiple ? Operator Usage: In JavaScript/TypeScript, use the optional chaining operator (?.) to avoid potential errors related to undefined values early. You can remove the outermost ??. Also ensure that all intermediate properties are defined.

2. Code Readability: The code can be made more readable by using proper indentation and spacing.

3. Direct Access Instead of Dot Notation: If set requires direct access, it might simplify if directly accessed instead of using object.assign or similar methods when setting property values like step name.

Here is the revised version of the modified function:

const editName = async (formEl: FormInstance | undefined) => {
  // Ensure formEl is properly typed and has validate method
  await (formEl as any).validate(async (valid) => {
    if (valid) {
      // Filter nodes excluding self-id and check uniqueness
      const updatedNodes = props.nodeModel.graphModel.nodes.filter(
        (node: any) => node.id !== props.nodeModel.id && node.properties.stepName === form.value.title
      );

      // Check existence
      if (!updatedNodes.length) {
        set(props.nodeModel.properties, 'stepName', form.value.title);
      }
      
      nodeNameDialogVisible.value = false;
    }
  });
};

Explanation of Changes:

• Removed the double question mark after nodes.filter(...) on multiple lines because TypeScript will automatically handle those cases correctly without needing explicit type assertions.
• Ensured proper typing before calling .filter() on graphModel.nodes.
• Simplified the logic within the filter condition for better readability. If no matching node is found, it sets the new title; otherwise, it doesn't change anything.