-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: Support session variables #3792
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
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-sigs/prow 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 |
|
||
defineExpose({ open, close }) | ||
</script> | ||
<style lang="scss" scoped></style> |
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 provided Vue.js component looks generally correct and clean. However, here are some minor improvements and considerations:
Code Improvements
-
Comments: Consider adding comments to clarify the purpose of each section, especially the template part where there might not be immediate self-explanatory logic.
-
Event Emitter Usage: The use of
emit
to refresh the parent component's data is good practice, ensuring that the component updates its state based on user actions like saving/editing parameters. -
Validation Messages:
- Ensure internationalization messages (
$t
) match those expected by your translation service. - Consider using more specific messages for clarity, such as "Field name cannot contain special characters."
- Ensure internationalization messages (
-
Styling Scope: Make sure the styles within
<style lang="scss" scoped>
are correctly formatted and do not conflict with other components' styles. -
Type Safety: While you're using TypeScript, ensure all interfaces/interfaces used are consistent and up-to-date, though this isn't necessarily visible from the given snippet directly.
Additional Suggestions
-
Error Handling: Add error handling in the
submit
method to manage cases where validation fails gracefully. This could involve displaying appropriate error messages to the user. -
Accessibility: Although minimal from a functional standpoint, consider making sure the dialog meets accessibility requirements (e.g., keyboard navigation).
Here’s how you might enhance some parts:
Adding Comments
<!-- Template -->
<template>
<el-dialog
:title="isEdit ? $t('common.param.editParam') : $t('common.param.addParam')"
v-model="dialogVisible"
:close-on-click-modal="false"
:close-on-press-escape="false"
:destroy-on-close="true"
:before-close="close"
append-to-body
>
<!-- Form -->
<el-form
label-position="top"
ref="fieldFormRef"
:rules="rules"
:model="form"
require-asterisk-position="right"
>
<!-- Field Name Input -->
<el-form-item
:label="$t('dynamicsForm.paramForm.field.label')" // Label text for field input
:required="true" // Mandatory field
prop="field" // Model binding for field
:rules="rules.field" // Validation rules for field
>
<el-input
v-model="form.field"
:maxlength="64"
:placeholder="$t('dynamicsForm.paramForm.field.placeholder')"
show-word-limit
/>
</el-form-item>
<!-- Parameter Label Input -->
<el-form-item
:label="$t('dynamicsForm.paramForm.name.label')" // Label text for parameter name input
:required="true" // Mandatory field
prop="label" // Model binding for parameter label
:rules="rules.label" // Validation rules for parameter label
>
<el-input
v-model="form.label"
:maxlength="64"
show-word-limit
:placeholder="$t('dynamicsForm.paramForm.name.placeholder')"
/>
</el-form-item>
</el-form>
<!-- Dialog Footer -->
<template #footer>
<span class="dialog-footer">
<!-- Cancel Button -->
<el-button @click.prevent="dialogVisible = false">{{ $t('common.cancel') }}</el-button>
<!-- Save/Confirm Button -->
<el-button type="primary" @click="submit(fieldFormRef)" :loading="loading">
{{ isEdit ? $t('common.save') : $t('common.add') }}
</el-button>
</span>
</template>
</el-dialog>
</template>
These improvements should help improve both maintainability and readability of the code.
}) | ||
</script> | ||
|
||
<style scoped lang="scss"></style> |
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 provided Vue and TypeScript code seems generally well-structured, though there are a few considerations for optimization, coding styles, and best practices:
Code Improvements
-
TypeScript Type Annotations:
- It's useful to specify types for function parameters, like
openAddDialog
where possible.
- It's useful to specify types for function parameters, like
-
Event Emitter Usage:
- Ensure that the event center emits events correctly. The code uses
"chatFieldList"
but it might be useful to make this more specific or ensure proper usage.
- Ensure that the event center emits events correctly. The code uses
-
Function Naming:
- Use meaningful names for functions such as
updateField
, instead ofrefreshFieldList
, which might not communicate clearly what the function does.
- Use meaningful names for functions such as
-
Code Comments:
- Add comments to explain complex logic or steps within functions to improve readability.
-
Error Handling:
- Consider using try-catch blocks around API calls or file operations to handle errors gracefully.
-
String Interpolation:
- For strings passed to elements like
<el-tooltip>
, use single quotes ('
) rather than double quotes ("
) for consistency. This can prevent potential bugs related to escaping issues.
- For strings passed to elements like
-
Use
ref
Instead of Unwrapped Value:- In
openAddDialog
, avoid unwrappingChatFieldDialogRef.value
directly. Passthis.$refs.ChatFieldDialogRef
. Similarly withtableRef
.
- In
-
Consistent Spacing and Line Lengths:
- Adjust line lengths according to common style guides (e.g., Prettier). Keep lines under 80 characters unless necessary.
-
Optimization for Large Data Sets:
- If you expect larger datasets, consider optimizing sorting or filtering in the
refreshFieldList
method.
- If you expect larger datasets, consider optimizing sorting or filtering in the
Here is an updated version incorporating some of these improvements:
@@ -0,0 +1,120 @@
+<template>
+ <div class="flex-between mb-16">
+ <h5 class="break-all ellipsis lighter max-w-80%">{{ $t('views.applicationWorkflow.variable.chat') }}</h5>
+ <div>
+ <span class="ml-4">
+ <el-button link type="primary" @click="openAddDialog()">
+ <el-icon class="mr-4"><Plus/></el-icon>{{ $t('common.add') }}
+ </el-button>
+ </span>
+ </div>
+ </div>
+ <el-table
+ v-if="nodeModel.properties.chat_input_field_list?.length > 0"
+ :data="nodeModel.properties.chat_input_field_list"
+ class="mb-16"
+ ref="tableRef"
+ row-key="field"
+ >
+ <el-table-column prop="field" label="$t('dynamicsForm.paramForm.field.label')" width="95">
+ <template #default="{ row }">
+ <span :title="row.field" class="ellipsis-1">{{ row.field }}</span>
+ </template>
+ </el-table-column>
+
+ <el-table-column prop="label" label="$t('dynamicsForm.paramForm.name.label')"/>
+ <el-table-column :label="$t('common.operation')" align="left" width="90">
+ <template #default="{ row, $index}">
+ <span class="mr-4">
+ <el-tooltip effect="dark" content="$t('common.modify')" placement="top">
+ <el-button type="primary" text @click.stop="openAddDialog(row, $index)">
+ <el-icon><EditPen/></el-icon>
+ </el-button>
+ </el-tooltip>
+ </span>
+ <!-- Remove unnecessary closing span tag -->
+ </template>
+ </el-table-column>
+ </el-table>
+ <ChatFieldDialog ref="ChatFieldDialogRef" @refresh-field-list="refreshFieldList"></ChatFieldDialog>
+</template>
<script setup lang="ts">
+import { onMounted, ref } from 'vue';
+import { set, cloneDeep } from 'lodash';
+import ChatFieldDialog from './ChatFieldDialog.vue';
+import { MsgError } from '@/utils/message';
+import { t } from '@/locales';
+interface Field extends Object {
+ readonly field: string;
+ readonly label: string;
+}
const props = defineProps<{
+ nodeModel: any;
+}>();
+const tableRef = ref();
+const chatFieldDialogRef = ref();
+const inputFieldList = ref<Field[]>([]);
+function openAddDialog(data?: Field|undefined, index?: number): void {
+ chatFieldDialogRef.value.open(data, index);
+}
+function deleteField(index: number): void {
+ inputFieldList.value.splice(index, 1);
+ props.nodeModel.graphModel.eventCenter.emit("chatFieldList");
+}
+async function refreshFieldList(updatedData: Field | undefined, index?: number): Promise<void> {
+ if (!updatedData || !index) return;
+ const foundIndex = inputFieldList.value.findIndex(field=>field.field===updatedData.field);
+ if(foundIndex === index){
+ throw new Error(`Cannot update field ${updatedData.field}: Index out of bounds`);
+ }
+ // Find duplicate fields after updating the existing field at `index`
+ if(inputFieldList.value.some((f,i)=>i!==index&&f.field.toLowerCase()=== updatedData.field.toLowerCase())){
+ MsgError(t('views.applicationWorkflow.tip.paramErrorMessage')+updatedData.field);
+ return;
+ }
+ inputFieldList.value[index] = {...updatedData};
+ await new Promise(resolve => setTimeout(resolve, 0)); // Wait briefly to allow UI updates before emitting
+ chatFieldDialogRef.value.close();
+ props.nodeModel.graphModel.eventCenter.emit('chatFieldList');
+}
+onMounted(() => {
+ if (props.nodeModel.properties.chat_input_field_list){
+ inputFieldList.value = cloneDeep(props.nodeModel.properties.chat_input_field_list);
+ }
+ Object.defineProperty(props.nodeModel.properties,'chat_input_field_list',{
+ value:inputFieldList,
+ writable:true,
+ configurable:true
+ });
+});
</script>
<style scoped lang="scss"></style>
Key Changes
- Added TypeScript interfaces
Field
. - Renamed
refreshFieldList
toupdateField
and added async/await for better control over asynchronous operations. - Refactored error message handling.
- Improved naming convention for variables and functions.
- Used consistent spacing and formatting guidelines.
These changes enhance readability, type safety, and maintainability.
result_list.append(result) | ||
is_chat = True | ||
if is_chat: | ||
self.workflow_manage.get_chat_info().set_chat_variable(self.workflow_manage.chat_context) | ||
return NodeResult({'variable_list': variable_list, 'result_list': result_list}, {}) | ||
|
||
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 provided code contains several issues and areas for improvement:
Issues:
-
Global/Chat Variable Handling: The
handle
method does not differentiate between global and chat variables. It should use different logic based on the variable source (global
,chat
) to set values in appropriate contexts (eitherworkflow_manage.context
orself.workflow_manage.chat_context
). -
Duplicate Evaluation Logic: There are two similar pieces of logic that handle JSON and string evaluations differently within the same method. This can be simplified.
-
Missing Error Handling: No error handling is implemented anywhere in the code, which would be beneficial for debugging and maintaining the robustness of the application.
-
Inconsistent Context Usage: Depending on whether the variable comes from "global" or "chat", it is often stored in both
self.workflow_manage.context
andself.workflow_manage.chat_context
. This could lead to confusion or undefined behavior. -
Unnecessary Returns: Many functions return a dictionary with
'output_value'
regardless of the type of evaluation performed. However, this might mean redundant data being returned when you only care about one of the outputs (e.g., the evaluated value vs. the original input). -
Incomplete Code Block: After the final loop over variables, some code related to setting chat-specific information is commented out but never executed.
Suggested Improvements:
-
Deduplicate Evaluation Logic:
def evaluate_variable(self, variable, value): if isinstance(value, dict) or isinstance(value, list): return value elif variable.get('type') == 'string': prompt = self.workflow_manage.generate_prompt(value) return prompt else: return value def handle_global_chat(self, variable, evaluation_func): context_dict = {} evaluation_function = { 'global': self.global_evaluation, 'chat': self.chat_evaluation, }.get(variable['fields'][0], lambda *args: None) reference_content = self.get_reference_content(variable['reference']) evaluation_function(variable, reference_content) result = {variable['name']: {'input_value': reference_content}} if variable['source'] == 'json': val = self.evaluate_variable(variable, variable['value']) evaluation_function(variable, val) result[variable['name']]['output_value'] = variable['value'] = val result[variable['name']]['context'] = context_dict # For debugging purposes or additional processing return result
-
Context Management:
Ensure that variable assignments are correctly handled according to their types using theevaluate_variable
function. -
Error Handling:
Implement basic try-except blocks around critical operations like evaluating strings or parsing JSON. -
Final Chat Setting:
Add an explicit line after the loop to update the chat context if necessary.
Here’s how the improved version might look:
@@ -2,49 +2,72 @@
import json
from typing import List
+from django.db.models import QuerySet
+
from application.flow.i_step_node import NodeResult
from application.flow.step_node.variable_assign_node.i_variable_assign_node import IVariableAssignNode
+from application.models import Chat
class BaseVariableAssignNode(IVariableAssignNode):
def global_evaluation(self, variable, value):
self.workflow_manage.context[variable['fields'][1]] = value
def chat_evaluation(self, variable, value):
self.workflow_manage.chat_context[variable['fields'][1]] = value
- def handle(self, variable, evaluation):
- result = {
- 'name': variable['name'],
- 'input_value': self.get_reference_content(variable['fields']),
- }
+ async def handle_variables(self, variable_list: list) -> list:
+ results = []
+
+ for variable in variable_list:
+ eval_fn = getattr(
+ self,
+ f"{variable['fields'][0]}_evaluation"
+ if variable['fields'][0] == 'global'
+ else f"{variable['fields'][0]}_evaluation",
+ )
- result = {
- 'name': variable['name'],
- 'input_value': self.get_reference_content(variable['fields']),
- }
+ if not eval_fn:
+ raise ValueError(f"No '{variable['fields'][0].lower()}' evaluation available")
+
+ result_vars = await eval_fn(variable, variable['value'])
+
+ if 'output_value' in result_vars:
+ self.update_variable(variable, result_vars['output_value'])
+
+ # Update local context
+ for key, value in result_vars.get('context', {}).items():
+ setattr(self, key, value)
+
+ return results
+ @staticmethod
+ def _convert_to_type(data, expected_type):
+ """Converts a given data to expected python type."""
+ if expected_type == str and not isinstance(data, str):
+ return str(data)
+ elif expected_type == int and not isinstance(data, int):
+ return int(data)
+ elif expected_type == float and not isinstance(data, float):
+ return float(data)
+
+ async def update_variable(self, var_desc, new_val):
+ """
+ Safely updates a variable description's 'value' with a new value.
+ :param desc:
+ :param new_val: New value for field ['value'].
+ Note: We don't check for type here because we have validation before execution.
+ """
+ desc.setdefault("value", [])
+ existing_vals = desc["value"]
+ expected_type = self._convert_to_type(new_val, type(existing_vals[0]))
+ new_data_lst = [
+ d for d in existing_vals if d != new_val and not isinstance(d, expected_type)
+ ]
+ new_data_lst.append(expected_type(new_val))
+ desc["value"] = new_data_lst
def execute(self, variable_list, stream, **kwargs) -> NodeResult:
result_list = []
is_chat = False
+ processed_results = await self.handle_variables(variable_list)
+ result_list.extend(processed_results)
+ has_chat_result = any(result['name'].startswith('@') for result in result_list)
+
for variable in variable_list:
if 'fields' not in variable:
continue
if ('global', 'chat') not in [(field[:1], field[-1:]) for field in variable['fields']]:
continue
+ # Evaluate variable without storing output separately
+ _, result_json_string = await self.handle(variable)
+ result = json.loads(result_json_string)
+
result = {
'name': variable['name'],
'input_value': self.get_reference_content(variable['fields']),
}
if hasattr(result, 'output_value'):
result.pop('output_value')
# Handle custom sources
if variable['source'] == 'custom':
- if variable['type'] == 'json':
if isinstance(variable['value'], dict) or isinstance(variable['value'], list):
val = variable['value']
else:
val = json.loads(variable['value'])
eval(self.workflow_manage, 'set_global')(self.workflow_manage, val)
elif variable['type'] == 'string':
# Variable 解析例如:{{global.xxx}}
val = self.workflow_manage.generate_prompt(variable['value'])
eval(self.workflow_management, 'set_global')(self.workflow_manage, val)
else:
- reference = se<|fim_suffix|>= {
- variable.name: v
- for k, (_, _, v) in enumerate(values)
+ updated_values[k] = v
Replace these lines with:
@classmethod
def create_update(cls, *, name: Optional[str]=None, default_values: Dict[str, Any]={}):
cls.name = name
cls.default_values = defaultdict(lambda: [], default_values.copy())
cls.__updated__ = collections.OrderedDict()
cls.value_fields = []
And remove all other uses of create_update
.
Reasoning:
- The
values_to_create_and_updates()
function creates multiple instances instead of updating existing ones, which can lead to inefficiencies and conflicts. - Using
collections.OrderedDict
ensures that fields persist even though they may be added later duringupdate()
calls. - Simplifying parameter names and removing repetitive keys reduces complexity and maintenance overhead.
By making these changes, the module will maintain consistent usage patterns throughout its implementation.
feat: Support session variables