Skip to content

Conversation

@shaohuzhang1
Copy link
Contributor

fix: Adjust the distance between the loop body and loop nodes

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Sep 23, 2025

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
Copy link

f2c-ci-robot bot commented Sep 23, 2025

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

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

@shaohuzhang1 shaohuzhang1 merged commit 070fb0f into v2 Sep 23, 2025
4 of 5 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@v2@fix_loop branch September 23, 2025 11:50
let y = props.nodeModel.y + 350
if (props.nodeModel.properties.node_data.loop_body) {
workflow = props.nodeModel.properties.node_data.loop_body
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code has the following issues:

  1. Variable Scope: The variable let workflow shadows the previously declared function parameter workflow.

  2. Incremental Y-Coordinate Addition: There is an inconsistency in how the y-coordinate is being incremented. The initial addition of 200 was changed to 350.

  3. Type Safety: Although not directly related to runtime errors, using any instead of more specific types can lead to errors if assumptions are incorrect at runtime.

  4. Logic for Loop Body Property: If nodeOutgoingNode already contains a node with the same type as loopBodyNode, setting workflow to props.nodeModel.properties.node_data.loop_body will overwrite the existing nodes without concatenating them.

Optimizations/Lessons Learned

  1. Avoid Variable Shadowing: Declare variables with unique names to avoid shadowing other variables within their scope.

  2. Consistent Code Style: Maintain consistency in incrementing the y-coordinate and ensure that there are no unintended changes in logic due to minor adjustments.

  3. Ensure Type Safety: Use appropriate TypeScript or JavaScript types to help catch potential errors related to data structures used.

  4. Handle Edge Cases: For properties like nodeData.loop_body, consider implementing checks to handle cases where the property might be missing or invalid before processing it.

Here is the corrected version of your code:

const mountLoopBodyNode = () => {
  const { nodeOutgoingNodes } = props;
  
  // Check if the outgoing edge matches the expected type
  if (!nodeOutgoingNodes.some(item => typeof item === 'object' && item.type === loopBodyNode.type)) {
    let workflow = {
      nodes: [loopStartNode],
      edges: []
    };

    const baseYOffset = 200; // Define base offset
    let yOffset = props.nodeModel.y;

    // Adjust based on previous condition (shouldn't run under this scenario, added just in case)
    if (baseYOffset !== 350) {
      yOffset += baseYOffset + 150; // Calculate actual desired offset
    }

    if (properties?.nodeData?.loop_body) {
      workflow.nodes.push(...(Array.isArray(properties?.nodeData?.loop_body) ? properties?.nodeData?.loop_body : [properties?.nodeData?.loop_body]));
    }
  }
};

In this updated version of the code:

  • Added strict typing (typeof item === 'object' &&) to better understand the structure of the items in nodeOutgoingNodes.
  • Removed the commented-out code related to changing the yOffset.
  • Ensured proper handling of array expansion when dealing with the loop_body property, assuming it's an array.
  • Used named objects for better readability and maintainability of the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants