Skip to content

Conversation

@shaohuzhang1
Copy link
Contributor

fix: Subsequent nodes in the form have lost reference data

@f2c-ci-robot
Copy link

f2c-ci-robot bot commented Oct 24, 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 Oct 24, 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

type: 'variable-aggregation-node',
model: AppNodeModel,
view: VariableAggregationNode,
}
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 provided code is mostly correct and does not contain significant irregularities or potential issues. However, there are a few optimizations you can consider:

  1. Simplify Constructors: In JavaScript/TypeScript, it's generally preferred to pass props directly to the parent class (AppNode) instead of creating a new object with them initially. This keeps things clear and concise.

  •  constructor(props: any) {
    
  •      super(props, VariableAggregationNodeVue);
    
  •  }       
    
  • constructor(props: any) {
  •    super(props);
    
  • }

2. **Remove Empty Line**: The newline at the end of the last line seems unnecessary and can be removed for better maintainability.

Here’s the updated version of your code:

```javascript
import VariableAggregationNodeVue from './index.vue'
import { AppNode, AppNodeModel } from '@/workflow/common/app-node'

class VariableAggregationNode extends AppNode {
constructor(props: any) {
 super(props)
}

getConfig(props: any) {
 return props.model.properties.config;
}
}

export default {
type: 'variable-aggregation-node',
model: AppNodeModel,
view: VariableAggregationNode,
};

This should make the code cleaner and more readable while maintaining its functionality.

}
getNodeName(nodes: Array<any>, baseName: string) {
let index = 0
let name = baseName
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No significant issue was found with the provided code. A couple of improvements could be made:

  1. Ensure the getConfig method is correctly exporting its results from Node.js.
  2. Add error handling in case props do not contain properties like 'type' or 'config'.
  3. Consider using strict type checking to improve readability and maintainability.

Overall, the original code seems straightforward and performs the task as intended.

model: AppNodeModel,
view: FormNode
view: FormNode,
}
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 provided code looks mostly correct in terms of syntax and intent. Here's a brief analysis:

  1. Getter Method: The getConfig method provides a consistent way to access the configuration from the props.model.properties.config. This is good practice if you need this functionality frequently within other methods.

  2. Export Statements: Both type, model, and view are correctly exported with their respective values.

  3. Constructor: The constructor remains straightforward, initializing the parent class (AppNode) with both props and FormNodeVue.

  4. No Immediate Issues Found: Apart from using camelCase consistently throughout (e.g., getConfig instead of GetConfig), there don't seem to be any significant problems with structure or logic.

If you have specific concerns or additional requirements not covered here, feel free to let me know!

@shaohuzhang1 shaohuzhang1 merged commit 5308214 into v2 Oct 24, 2025
4 of 6 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@v2@fix_workflow branch October 24, 2025 06:49
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