Skip to content

Refactoring Validation logic into separate function#6272

Merged
abdullah-workday merged 1 commit intomainfrom
refactoring-validation
Apr 22, 2026
Merged

Refactoring Validation logic into separate function#6272
abdullah-workday merged 1 commit intomainfrom
refactoring-validation

Conversation

@abdullah-workday
Copy link
Copy Markdown
Contributor

Overview

  • Refactoring packages/server/src/services/validation/index.ts to extract the validation logic into a separate function.

What is Changed

  • packages/server/src/services/validation/index.ts -> Code Refactoring
  • packages/server/src/services/validation/validateFlowData.test.ts -> Adding unit test

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the flow validation logic by extracting the core validation rules into a pure function, validateFlowData, and introduces a comprehensive unit test suite. The refactoring improves testability and separates data retrieval from validation logic. Feedback identifies a performance bottleneck in the hanging edges check that could be optimized using a Set for node lookups. Additionally, a logic issue was found where the regex for normalizing array index conditions is hardcoded to a specific parameter name, which may cause validation to fail for other array-type parameters.

Comment on lines +251 to +254
for (const edge of edges) {
const sourceExists = nodes.some((node: IReactFlowNode) => node.id === edge.source)
const targetExists = nodes.some((node: IReactFlowNode) => node.id === edge.target)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The hanging edges check has $O(E \times N)$ complexity because it performs nodes.some (an $O(N)$ operation) twice for every edge. For large flows, this can significantly impact performance. Using a Set for node ID lookups reduces this to $O(E)$.

    // Check for hanging edges
    const nodeIds = new Set(nodes.map((node) => node.id))
    for (const edge of edges) {
        const sourceExists = nodeIds.has(edge.source)
        const targetExists = nodeIds.has(edge.target)


if (isIndexCondition) {
// Replace $index with actual index and evaluate
const normalizedKey = conditionKey.replace(/conditions\[\$index\]\.(\w+)/, '$1')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The regex for $index conditions is hardcoded to the string conditions. If an array parameter has a different name (e.g., actions), this validation logic will fail to correctly normalize the key. This issue also exists on line 135. Consider using the actual parameter name param.name to build the regex dynamically.

@abdullah-workday abdullah-workday merged commit 34ab4e0 into main Apr 22, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants