-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
feat: enhance form handling with dynamic input fields and reference content retrieval #2686
Conversation
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/test-infra repository. |
[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:
Approvers can indicate their approval by writing |
} | ||
console.log(form_data.value.tool_params) | ||
} | ||
|
||
const form_data = computed({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem to be several issues with the provided Vue component. Here are some key points:
-
Duplicate
dynamicsFormRef
: Both instances of<DynamicsForm>
have the same reference (dynamicsFormRef
). This can lead to conflicts or unexpected behavior. -
Vue Composition API Misuse: The code uses the Vue Composition API effectively but could use more comments and clarity for better understanding.
-
Dynamic Form Rendering Issues:
- When
form_data.value.params_nested
exists, a new object{ [form_data.value.params_nested]: {} }
is created and set asform_data.value.tool_params
. However, this might not be intended. - Calling
dynamicsFormRef.value?.render
seems problematic becausedynamicsFormRef
hasn’t been properly initialized yet at this point in the lifecycle.
- When
-
Lack of Error Handling: There’s no error handling in place for cases where data fetching fails, such as when calling
applicationApi.getApplication
. -
Component Imports:
TooltipLabel
andNodeCascader
should also come from their respective directories.
-
Conditional Class Usage:
- Use proper conditional classes instead of relying on inline styles like
lighter
to ensure maintainability and reusability.
- Use proper conditional classes instead of relying on inline styles like
Here’s an updated version with corrections:
<template>
<NodeContainer :style="{ minHeight: `calc(100% - ${props.nodeModel.height}px)`}">
<!-- Nested Tool Parameters Panel -->
<div
v-show="form_data.tool_params[form_data.params_nested]"
class="border-r-4 p-8-12 mb-8 layout-bg lighter"
>
<el-form
ref="nodeFormNestedRef"
label-position="top"
v-loading="loading"
require-asterisk-position="right"
:hide-required-asterisk="true"
@submit.prevent.stop="handleSubmitNested"
>
<el-form-item v-for="(item, itemKey) in form_data.tool_form_field"
:key="item.field"
:rules="[...applyRules(item.props_info.rules), validateRequired(item)]">
<template #label>
<div class="flex-between">
<div>
<TooltipLabel :label="item.label.label" :tooltip="item.label.attrs.tooltip" />
<span v-if="item.required" class="danger">*</span>
</div>
<el-select :teleported="false" v-model="item.source" size="small" style="width: 85px">
<el-option
label="$t('views.applicationWorkflow.nodes.replyNode.replyContent.reference')"
value="referencing"
/>
<el-option
label="$t('views.applicationWorkflow.nodes.replyNode.replyContent.custom')"
value="custom"
/>
</el-select>
</div>
</template>
<el-input
v-if="item.source === 'custom'"
v-model="form_data.tool_params[form_data.params_nested][item.label.label]"
/>
<NodeCascader
v-else
ref="nodeCascaderRef"
:nodeModel="nodeModel"
class="w-full"
:placeholder="$t('views.applicationWorkflow.variable.placeholder')"
v-model="form_data.tool_params[form_data.params_nested][item.label.label]"
/>
</el-form-item>
</el-form>
</div>
<!-- Regular Tool Parameters Panel -->
<div v-else class="border-r-4 p-8-12 mb-8 layout-bg lighter">
<el-form
ref="nodFormRegularRef"
label-position="top"
v-loading="loading"
require-asterisk-position="right"
:hide-required-asterisk="true"
@submit.prevent.stop="handleSubmitRegular"
>
<el-form-item v-for="(item, itemKey) in form_data.tool_form_field"
:key="item.field"
:rules="[…applyRules(item.props_info.rules), validateRequired(item)]">
<template #label>
<div class="flex-between">
<div>
<TooltipLabel :label="item.label.label" :tooltip="item.label.attrs.tooltip" />
<span v-if="item.required" class="danger">*</span>
</div>
<el-select :teleported="false" v-model="item.source" size="small" style="width: 85px">
<el-option
label="$t('views.applicationWorkflow.nodes.replyNode.replyContent.reference')"
value="referencing"
/>
<el-option
label="$t('views.applicationWorkflow.nodes.replyNode.replyContent.custom')"
value="custom"
/>
</el-select>
</div>
</template>
<el-input
v-if="item.source === 'custom'"
v-model="form_data.tool_params[item.label.label]"
/>
<NodeCascader
v-else
ref="nodeCascaderRef"
:nodeModel="nodeModel"
class="w-full"
:placeholder="$t('views.applicationWorkflow.variable.placeholder')"
v-model="form_data.tool_params[item.label.label]"
/>
</el-form-item>
</el-form>
</div>
</NodeContainer>
</template>
<script lang="ts">
import { Component, Prop, Watch, ComputedRef, Ref } from 'vue';
import NodeContainer from '@/workflow/common/NodeContainer.vue';
import TooltipLabel from '@/components/dynamics-form/items/label/TooltipLabel.vue';
import NodeCascader from '@/workflow/common/NodeCascader.vue';
import * as moment from 'moment';
import { AxiosInstance } from 'axios';
@Component({
components: {
NodeContainer,
TooltipLabel,
NodeCascader
}
})
export default class YourComponentName implements Options {
@Prop({ required: true }) readonly nodeModel!: any;
// ... rest of the code remains largely similar ...
private handleChangeTool() {
if (this.form_data.value.params_nested && !Object.keys(this.form_data.value.tool_params).includes(this.form_data.value.params_nested)) {
this.form_data.value.tool_params[this.form_data.value.params_nested] = {};
// Assuming renderData is already populated elsewhere
this.dynamicsFormNestedRef.render(this.form_data.value.tool_form_field, this.form_data.value.tool_params[this.form_data.value.params_nested]);
} else {
this.form_data.value.tool_params = {};
}
// console.log(this.form_data.value.tool_params);
}
handleSubmitNested(event: Event) {
event.preventDefault();
// Handle nested tool parameters submission logic here
console.log('Submitting nested form:', this.form_data.value.tool_params);
}
handleSubmitRegular(event: Event) {
event.preventDefault();
// Handle regular tool parameters submission logic here
console.log('Submitting regular form:', this.form_data.value.tool_params);
}
applyRules(rules?: Record<string, any>[]): Rule[] {
return rules ? Object.values(rules) : [];
}
validateRequired(item: any): Rule | null {
return !(Boolean(item.required) || !!item.default_value);
}
// Initialize refs outside before usage
@Ref<NodeFormComponent>() readonly dynamicsFormNestedRef!: InstanceType<typeof Element>;
@Ref<NodeFormComponent>() readonly nodFormRegularRef!: InstanceType<typeof Element>;
mounted() {
// Perform any setup after mounting goes here
}
}
Make sure that the imported components and properties exist according to their paths used in the template. Also, replace placeholders and method names as per actual implementation details. If you still experience errors after making these changes, double-check all dependencies and imports are correctly installed and configured.
@@ -35,6 +35,8 @@ def handle_variables(self, tool_params): | |||
tool_params[k] = self.workflow_manage.generate_prompt(tool_params[k]) | |||
if type(v) == dict: | |||
self.handle_variables(v) | |||
if (type(v) == list) and (type(v[0]) == str): | |||
tool_params[k] = self.get_reference_content(v) | |||
return tool_params | |||
|
|||
def get_reference_content(self, fields: List[str]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks generally clean but has a few optimizations that could be applied:
-
Check for None in
tool_params
: You might want to add a check at the top of the method to see iftool_params
is not None before proceeding to iterate over it. -
Early Exit Check for Dictionaries: If you know that only non-dictionary values should undergo further processing, you can add an early exit after handling dictionaries.
def handle_variables(self, tool_params):
if tool_params is None:
raise ValueError("tool_params must be provided")
for k, v in tool_params.items():
if type(v) == dict:
self.handle_variables(v)
+ elif (type(v) == list) and all(isinstance(item, str) for item in v): # Added check for list of strings
+ tool_params[k] = self.get_reference_content(v)
else: # Other types don't need further processing in this context
pass
return tool_params
def get_reference_content(self, fields: List[str]):
These changes ensure that the function handles inputs more robustly and skips unnecessary steps when dealing with dictionary lists.
feat: enhance form handling with dynamic input fields and reference content retrieval