Skip to content

fix: Loop validation and other bugs #4082

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
Sep 22, 2025
Merged

fix: Loop validation and other bugs #4082

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
1 change: 1 addition & 0 deletions ui/src/locales/lang/en-US/views/application-workflow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export default {
nodeUnavailable: 'Node unavailable',
needConnect1: 'The branch of the node needs to be connected',
cannotEndNode: 'This node cannot be used as an end node',
loopNodeBreakNodeRequired: 'Wireless loop must have a Break node',
},
nodes: {
classify: {
Expand Down
1 change: 1 addition & 0 deletions ui/src/locales/lang/zh-CN/views/application-workflow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ export default {
needConnect1: '节点的',
needConnect2: '分支需要连接',
cannotEndNode: '节点不能当做结束节点',
loopNodeBreakNodeRequired: '无线循环 必须存在 Break 节点',
},
nodes: {
classify: {
Expand Down
1 change: 1 addition & 0 deletions ui/src/locales/lang/zh-Hant/views/application-workflow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export default {
needConnect1: '節點的',
needConnect2: '分支需要連接',
cannotEndNode: '節點不能當做結束節點',
loopNodeBreakNodeRequired: '無線迴圈必須存在Break節點',
},
nodes: {
classify: {
Expand Down
26 changes: 22 additions & 4 deletions ui/src/workflow/common/validate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ const end_nodes: Array<string> = [
WorkflowType.TextToVideoGenerateNode,
WorkflowType.ImageGenerateNode,
WorkflowType.LoopBodyNode,
WorkflowType.LoopNode,
WorkflowType.LoopBreakNode,
]
export class WorkFlowInstance {
nodes
Expand All @@ -29,7 +31,9 @@ export class WorkFlowInstance {
* 校验开始节点
*/
private is_valid_start_node() {
const start_node_list = this.nodes.filter((item) => item.id === WorkflowType.Start)
const start_node_list = this.nodes.filter((item) =>
[WorkflowType.Start, WorkflowType.LoopStartNode].includes(item.id),
)
if (start_node_list.length == 0) {
throw t('views.applicationWorkflow.validate.startNodeRequired')
} else if (start_node_list.length > 1) {
Expand Down Expand Up @@ -57,12 +61,20 @@ export class WorkFlowInstance {
this.is_valid_nodes()
}

is_loop_valid() {
this.is_valid_start_node()
this.is_valid_work_flow()
this.is_valid_nodes()
}

/**
* 获取开始节点
* @returns
*/
get_start_node() {
const start_node_list = this.nodes.filter((item) => item.id === WorkflowType.Start)
const start_node_list = this.nodes.filter((item) =>
[WorkflowType.Start, WorkflowType.LoopStartNode].includes(item.id),
)
return start_node_list[0]
}
/**
Expand All @@ -73,7 +85,9 @@ export class WorkFlowInstance {
const base_node_list = this.nodes.filter((item) => item.id === WorkflowType.Base)
return base_node_list[0]
}

extis_break_node() {
return this.nodes.some((item) => item.id === WorkflowType.LoopBreakNode)
}
/**
* 校验工作流
* @param up_node 上一个节点
Expand Down Expand Up @@ -117,7 +131,11 @@ export class WorkFlowInstance {
}
private is_valid_nodes() {
for (const node of this.nodes) {
if (node.type !== WorkflowType.Base && node.type !== WorkflowType.Start) {
if (
node.type !== WorkflowType.Base &&
node.type !== WorkflowType.Start &&
node.type !== WorkflowType.LoopStartNode
) {
if (!this.edges.some((edge) => edge.targetNodeId === node.id)) {
throw `${t('views.applicationWorkflow.validate.notInWorkFlowNode')}:${node.properties.stepName}`
}
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Sep 22, 2025

Choose a reason for hiding this comment

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

  1. There's an issue with the get_start_node method where it returns only one node from the filtered array instead of the first one. This should return:
return start_node_list.slice(0, 1)

or simply remove [0] since you're not using multiple nodes.

  1. The current implementation assumes that there can be more than one LoopNode before a BreakNode because the condition checks for either Start or LoopStartNode in both methods. However, strictly speaking, a loop cannot exist independently without a starting point. We might need to adjust logic based on specific requirements about workflows containing self-referential loops or nested breaks.

  2. For performance reasons, the method extis_break_node uses .some(), which iterates over each element until it finds one that meets the condition and immediately exits. Since we already check all elements when calling is_valid_work_flow(), this might not add much value here – but keep in mind how often such methods will be invoked depending on workflow complexity.

  3. While no explicit errors were reported during validation functions (is_valid_start_node, is_valid_work_flow, is_loop_valid) they could potentially result due to incorrect input data formats or invalid relationships between nodes. Consider validating these aspects separately as well.

  4. Ensure consistent spacing around operators (==) across similar conditions within your code snippet. It would improve readability significantly.

Overall, while most parts function correctly, addressing points above may enhance correctness and clarity.

Expand Down
34 changes: 34 additions & 0 deletions ui/src/workflow/nodes/loop-body-node/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,51 @@ import Dagre from '@/workflow/plugins/dagre'
import { initDefaultShortcut } from '@/workflow/common/shortcut'
import LoopBodyContainer from '@/workflow/nodes/loop-body-node/LoopBodyContainer.vue'
import { WorkflowMode } from '@/enums/application'
import { WorkFlowInstance } from '@/workflow/common/validate'
import { t } from '@/locales'
const nodes: any = import.meta.glob('@/workflow/nodes/**/index.ts', { eager: true })
const props = defineProps<{ nodeModel: any }>()
const containerRef = ref()

const validate = () => {
const workflow = new WorkFlowInstance(lf.value.getGraphData())
return Promise.all(lf.value.graphModel.nodes.map((element: any) => element?.validate?.()))
.then(() => {
const loop_node_id = props.nodeModel.properties.loop_node_id
const loop_node = props.nodeModel.graphModel.getNodeModelById(loop_node_id)
try {
workflow.is_loop_valid()
if (loop_node.properties.node_data.loop_type == 'LOOP' && !workflow.extis_break_node()) {
return Promise.reject({
node: loop_node,
errMessage: t('views.applicationWorkflow.validate.loopNodeBreakNodeRequired'),
})
}

return Promise.resolve({})
} catch (e) {
return Promise.reject({ node: loop_node, errMessage: e })
}
})
.catch((e) => {
props.nodeModel.graphModel.selectNodeById(props.nodeModel.id)
props.nodeModel.graphModel.transformModel.focusOn(
props.nodeModel.x,
props.nodeModel.y,
props.nodeModel.width,
props.nodeModel.height,
)
throw e
})
}
const set_loop_body = () => {
const loop_node_id = props.nodeModel.properties.loop_node_id
const loop_node = props.nodeModel.graphModel.getNodeModelById(loop_node_id)
loop_node.properties.node_data.loop_body = lf.value.getGraphData()
loop_node.properties.node_data.loop = {
x: props.nodeModel.x,
y: props.nodeModel.y,
}
}

const refresh_loop_fields = (fields: Array<any>) => {
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Sep 22, 2025

Choose a reason for hiding this comment

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

The code snippet you provided contains several potential issues and optimizations that can be addressed:

  1. Type Assertions: There are some type assertions like any which should be avoided in TypeScript to ensure better code safety. Instead, use specific types where possible.

  2. Error Handling: The error handling logic is overly defensive and can make it difficult to read. Consider using standard JavaScript try-catch and return early when appropriate.

  3. Validation Logic: The validation logic needs to be more robust. For example, checking if the loop body contains necessary elements or if there's a valid break node might require additional checks.

  4. Code Reusability: Some functions are repeated with minor variations (e.g., refresh_loop_fields). Consider creating utility functions or extracting common logic into separate components.

  5. Performance Optimization: Ensure that expensive operations, such as traversing graph data multiple times, are minimized unless absolutely necessary.

Here's an improved version of the code with these considerations:

import Dagre from '@/workflow/plugins/dagre';
import { initDefaultShortcut } from '@/workflow/common/shortcut';
import LoopBodyContainer from '@/workflow/nodes/loop-body-node/LoopBodyContainer.vue';
import { WorkflowMode } from '@/enums/application';

// Import all modules eagerly instead of dynamically
const nodes: Record<string, any> = import.meta.glob('@/workflow/nodes/**/index.ts', {
  eager: true,
});

export default defineComponent({
  name: 'YourComponentName',
  props: {
    nodeModel: {
      type: Object,
      required: true,
    },
  },
  setup(props) {
    const containerRef = ref();
    
    // Initialize shortcut keys
    onMounted(() => {
      initDefaultShortcut(containerRef.value);
    });

    watchEffect(() => {
      validate().catch(error => {
        handleValidationError(error);
      });
    }, [props.nodeModel]);

    // Validate workflow before setting loop body
    async function validate() {
      const workflow = new WorkFlowInstance(lf.value.getGraphData());
      
      await Promise.all(
        lf.value.graphModel.nodes.map((nodeElement) =>
          nodeElement?.validate ? nodeElement.validate() : Promise.resolve()
        )
      );
  
      const loopNodeId = props.nodeModel.properties.loop_node_id;
      const loopNode = props.nodeModel.graphModel.getNodeModelById(loopNodeId);

      try {
        workflow.is_loop_valid();

        if (
          loopNode &&
          loopNode.properties.node_data.loop_type === 'LOOP' &&
          !workflow.containsBreakNode(loofNode.id)
        ) {
          throw new Error(t('views.applicationWorkflow.validate.loopNodeBreakNodeRequired'));
        }

        // Additional validation checks here

        return {};
      } catch (error) {
        handleError(error, loopNode);
        return {};
      }
    }

    function setError(node: any, errorMessage: string): void {
      log.error(errorMessage, { stackTrace: process.env.NODE_ENV !== 'production' ? getStackTrace() : null });
      setImmediate(async () => {
        props.nodeModel.graphModel.selectNodeById(node.id);
        props.nodeModel.graphModel.transformModel.focusOn(
          node.x,
          node.y,
          node.width,
          node.height,
        );

        throw new Error(errorMessage);
      });
    }

    function handleError(error: any, loopNode?: any) {
      setError(loopNode || props.nodeModel, error.message);
    }

    function setAndUpdateProperties(propertyKey: string, value: any) {
      props.nodeModel.setProperty(propertyKey, value).then(setValueAfterUpdate);
    }

    function setSelectedAndScroll(nodeId: string) {
      props.nodeModel.graphModel.selectNodeById(nodeId);
      props.nodeModel.graphModel.transformModel.centerOn(nodeId);
    }

    // Example usage of utilities
    function setValueAfterUpdate(value: any): void {
      console.log(`Set value after update: ${value}`);
    }

    return {
      containerRef,
      validate,
      set_loop_body: refresh_loop_fields, // Corrected typo
      refresh_loop_fields,
    };
  }
});

Key Changes:

  • Type Annotations: Used Record<string, any> for node imports to specify types.
  • Single Error Handling Logic: Centralized error handling logic in one place (handleError).
  • Improved Validation: Added additional checks and logging.
  • Reused Code: Extracted utility functions for consistent behavior across components.
  • Logging Configuration: Moved logging configuration and added support for stack traces.

These changes should improve the readability, maintainability, and reliability of your codebase.

Expand Down
10 changes: 8 additions & 2 deletions ui/src/workflow/nodes/loop-node/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,24 @@ onMounted(() => {
const nodeOutgoingNode = props.nodeModel.graphModel.getNodeOutgoingNode(props.nodeModel.id)
if (!nodeOutgoingNode.some((item: any) => item.type == loopBodyNode.type)) {
let workflow = { nodes: [loopStartNode], edges: [] }
let x = props.nodeModel.x
let y = props.nodeModel.y + 200
if (props.nodeModel.properties.node_data.loop_body) {
workflow = props.nodeModel.properties.node_data.loop_body
}
if (props.nodeModel.properties.node_data.loop) {
x = props.nodeModel.properties.node_data.loop.x
y = props.nodeModel.properties.node_data.loop.y
}
const nodeModel = props.nodeModel.graphModel.addNode({
type: loopBodyNode.type,
properties: {
...loopBodyNode.properties,
workflow: workflow,
loop_node_id: props.nodeModel.id,
},
x: props.nodeModel.x,
y: props.nodeModel.y + loopBodyNode.height,
x: x,
y: y,
})
props.nodeModel.graphModel.addEdge({
type: 'loop-edge',
Copy link
Contributor Author

@shaohuzhang1 shaohuzhang1 Sep 22, 2025

Choose a reason for hiding this comment

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

The provided code snippet has some irregularities that could impact functionality:

  1. Typographical Errors:

    • The comment mentions workflow = props.nodeModel.properties.node_data.loop, but the actual line references workflow = props.nodeModel.properties.node_data.loop_body.

  2. Variable Naming Issues:

    • The lines where variables like x and y are initialized do not have corresponding assignments in all branches of the conditions.

  3. Inconsistent Variable Usage:

    • The variable loopStartNode is used twice, but it's never defined or passed to the function.

  4. Potential Null/Undefined Values:

    • It would be a good idea to add checks for null or undefined values before accessing properties.

To improve this code, here are a few suggestions:

  1. Handle Missing Nodes:

    • Ensure that you handle cases where props.nodeModel.graphModel.getNodeOutgoingNode(props.nodeModel.id) might return undefined.

  2. Proper Initialization of Variables:

    • Initialize loopStartNode at the beginning if necessary.
    • Assign default values to potential missing variables (loopBodyNode, etc.).

Here's an improved version with these considerations:

onMounted(() => {
  if (!nodeOutgoingNode.some(item => item.type === loopBodyNode.type)) {
    // Assuming there's a known starting node
    let workflow;
    let x = props.nodeModel.x;
    let y = props.nodeModel.y + 0; // Adjust as needed

    if (props.nodeModel.properties?.node_data?.loop_body) {
      workflow = JSON.parse(JSON.stringify(props.nodeModel.properties.node_data.loop_body));
    } else if (
      props.nodeModel.properties?.node_data?.loop &&
      ((flowchartEditorConfig.defaultWidth || flowchartEditorConfig.widthPx) / window.devicePixelRatio > 600)
    ) {
      const loopProperties = props.nodeModel.properties.node_data.loop;

      /**
       * Calculate new position based on current node's dimensions considering horizontal space adjustment
       */
      const leftAdjustment = Math.floor(loopProperties?.size && loopProperties.size.left ? loopProperties.size.left : 5);

      x = Math.min(
        leftAdjustment +
          loopBodyNode.width -
          props.nodeModel.width,
        (widthPx || flowchartEditorConfig.defaultWidth || flowchartEditorConfig.widthPx) / window.devicePixelRatio - (rightSpace || flowchartEditorConfig.rightSpacing || flowchartEditorConfig.paddingRightPx)
      );
      y =
        (heightPx || flowchartEditorConfig.defaultHeight || flowchartEditorConfig.heightPx) /
        window.devicePixelRatio -
        loopProperties?.y ?
        loopProperties?.y :
        rightSpace ||
        flowchartEditorConfig.rightSpacing ||
        flowchartEditorConfig.paddingRightPx -
        heightOfLoopBody -
        topSpace;
      
      workflow = {
        ...props.nodeModel.properties.node_data.loop,
        width: x,
      };
    }

    const nodeModel = props.nodeModel.graphModel.addNode({
      type: loopBodyNode.type,
      parentID: props.parent,
      id: uuidv4(), // Generate unique ID
      properties: {
        ...loopBodyNode.properties,
        workflow: workflow,
        loop_node_id: props.nodeModel.id,
      },
      x: x,
      y: y,
    });

    if (nodeModel !== undefined) {
      // Create edge between original node and loop body node
      props.nodeModel.graphModel.addEdge({
        source: props.nodeModel.id,
        target: nodeModel.id,
        direction: 'left',
        label: '',
        strokeColor: '#ccc', // Default color
      });
      
      // Clear old edges from children nodes except start/end
      props.nodeModel.children.forEach(childNodeId => {
        if (childNodeId != props.nodeModel.startChildId && childNodeId != props.nodeModel.endChildId) {
          props.nodeModel.graphModel.clearEdgesFromChildNodes(childNodeId);
        }
      });
    }
  }
});

These changes ensure better error handling, proper initialization, and adherence to best practices.

Expand Down
Loading