[REF-1257] feat: enhance WorkflowRunForm with improved validation and file handling#2104
[REF-1257] feat: enhance WorkflowRunForm with improved validation and file handling#2104
Conversation
- Added new components for rendering form fields based on variable types, including string, option, and resource. - Implemented validation logic to ensure required fields are filled before submission. - Introduced file upload handling for resource type variables, including upload, removal, and refresh functionalities. - Enhanced user feedback for form validation errors and improved overall user experience with Tailwind CSS styling. - Utilized useMemo and useCallback for performance optimizations and to maintain referential equality in function props.
📝 WalkthroughWalkthroughRefactors workflow-run-form to render an in-component Form, manage variable state/validation internally (including template-driven variables), add resource file upload flows (with optional DriveFile creation), and rework pre-run checks (login, tool dependencies, credits) and submission lifecycle. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form as WorkflowRunForm
participant Upload as ResourceUpload
participant API as DriveFile API
participant State as Local State
User->>Form: Open form / interact
Form->>Upload: Render ResourceUpload for resource variable
User->>Upload: Select file
Upload->>Upload: handleFileUpload()
Upload->>API: Upload / create DriveFile (optional)
API-->>Upload: fileId / response
Upload->>State: Store storageKey and optional fileId
State-->>Form: Update variable value
Form->>User: Show file preview / controls
sequenceDiagram
participant User
participant Form as WorkflowRunForm
participant Validator as Validation Logic
participant Auth as Auth Service
participant Tools as ToolsDependencyChecker
participant Runner as Workflow Engine
User->>Form: Click Run
Form->>Form: set attemptedSubmit = true
Form->>Validator: Validate (templateVariables or form fields)
Validator-->>Form: Validation result
alt Validation fails
Form->>User: Display field errors
else
Form->>Auth: Check login status
Auth-->>Form: Login state
alt Not logged in
Form->>User: Show login modal
else
Form->>Tools: Check tool dependencies
Tools-->>Form: Dependencies OK / missing
alt Missing dependencies
Tools->>User: Show dependency UI
else
Form->>Runner: Execute workflow with converted variables
Runner-->>Form: Execution result
Form->>User: Show run status/result
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…s within the input parameter row component.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx`:
- Around line 222-244: The current validation in isFormValid can throw if value
isn't a string and treats unknown variable types as invalid while duplicating
logic in getInvalidFields; extract the per-variable validation into a shared
helper (e.g., validateVariable(variable, variableValues)) and use it from both
isFormValid and getInvalidFields, ensure the helper checks typeof value ===
'string' before calling trim for variable.variableType === 'string', handle
'option' and 'resource' as before, and for unrecognized variable.variableType
either return true or call a logger/warn then return true so new types don't
break validation.
- Around line 379-433: The handleFileUpload useCallback references
handleValueChange and form but they are missing from its dependency array;
update the dependency array for handleFileUpload to include handleValueChange
and form (in addition to uploadFile, variableValues, canvasId,
workflowVariables) so React's hook rules are satisfied—ensure handleValueChange
is memoized with useCallback where it's defined before adding it, and then add
both handleValueChange and form to the dependency list of handleFileUpload.
🧹 Nitpick comments (9)
packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx (9)
58-65: Consider wrapping withReact.memo.Per coding guidelines, pure presentational components should be wrapped with
React.memoto prevent unnecessary re-renders. SinceRequiredTagTexthas no props and only uses a translation hook, it will re-render on every parent render.Suggested improvement
-const RequiredTagText = () => { +const RequiredTagText = React.memo(() => { const { t } = useTranslation(); return ( <div className="flex-shrink-0 text-[10px] text-refly-text-2 leading-[16px] px-1 border-[1px] border-solid border-refly-Card-Border rounded-[4px]"> {t('canvas.workflow.variables.required') || 'Required'} </div> ); -}; +});
85-98: Wrap withReact.memoand define explicit props type.Per coding guidelines, pure components should be wrapped with
React.memo. Also, props should be explicitly typed with an interface.Suggested improvement
+interface FormItemLabelProps { + name: string; + required: boolean; +} + -const FormItemLabel = ({ name, required }: { name: string; required: boolean }) => { +const FormItemLabel = React.memo(({ name, required }: FormItemLabelProps) => { return ( <div className="flex items-center gap-2 min-w-0"> <Typography.Paragraph ellipsis={{ rows: 1, tooltip: <div className="max-h-[200px] overflow-y-auto">{name}</div> }} className="!m-0 text-xs font-semibold text-refly-text-0 leading-4 max-w-[100px]" > {name} </Typography.Paragraph> {required && <RequiredTagText />} </div> ); -}; +});
370-376: WraphandleValueChangewithuseCallback.Per coding guidelines, function props should use
useCallbackto maintain referential equality. This function is passed to form inputs and will cause unnecessary re-renders on every parent render.Suggested fix
- const handleValueChange = (variableName: string, value: any) => { + const handleValueChange = useCallback((variableName: string, value: any) => { setVariableValues((prev) => ({ ...prev, [variableName]: value, })); - }; + }, []);
498-503: Consider addingformto the dependency array.While
formfromForm.useForm()is stable, including it in the dependency array follows best practices and satisfies exhaustive-deps lint rules.Suggested fix
- }, [workflowVariables, convertVariableToFormValue]); + }, [workflowVariables, convertVariableToFormValue, form]);
692-806: Inline object creation in render causes unnecessary re-renders.The
rulesarrays (lines 707-711, 742-746, 777-781) and inlineonChangehandlers are recreated on every render. Consider memoizing the rules or extracting them as constants. Also, consider wrappingrenderFormFieldwithuseCallbackfor potential optimization.Example: Extract rules as constants
// Outside the component or memoized inside const createRequiredRule = (message: string) => [{ required: true, message }]; // Then use: rules={required ? createRequiredRule(t('canvas.workflow.variables.inputPlaceholder')) : []}Alternatively, memoize the entire
renderFormFieldfunction:const renderFormField = useCallback((variable: WorkflowVariable) => { // ... existing implementation }, [variableValues, isFieldInvalid, handleValueChange, isFormDisabled, uploading, t, form, handleFileUpload, handleFileRemove, handleRefreshFile]);
819-844: Dead code: Unreachable branch due to constantfalsecondition.The
{false ? (condition (line 821) means the entireMixedTextEditorblock (lines 822-844) is unreachable. This appears to be intentional based on the biome-ignore comment, but keeping dead code increases maintenance burden.Consider removing this block if it's not needed, or use a feature flag/environment variable if it's intended for future use.
630-681: Fragile CSS selector and magic number.
Line 649: The hardcoded CSS selector
'.p-4.flex-1.overflow-y-auto'is fragile. If styling changes, the scroll-to-error functionality will silently fail. Consider using arefordata-*attribute instead.Line 670: The magic number
400for the timeout should be a named constant.Line 632: The error object lacks proper typing. Consider adding a type guard or interface for the form validation error.
Suggested improvements
// Define constant const FOCUS_DELAY_MS = 400; // Use ref or data attribute for scroll container const scrollContainerRef = useRef<HTMLDivElement>(null); // In JSX: <div ref={scrollContainerRef} className="p-3 sm:p-4 flex-1 overflow-y-auto"> // Type guard for form error interface FormValidationError { errorFields?: Array<{ name: (string | number)[]; errors: string[] }>; } const isFormValidationError = (error: unknown): error is FormValidationError => { return typeof error === 'object' && error !== null && 'errorFields' in error; }; // In catch block: if (isFormValidationError(error) && error.errorFields?.length) { // ... }
117-130: Consider wrappingWorkflowRunFormwithReact.memo.Per coding guidelines, components should be wrapped with
React.memoto prevent unnecessary re-renders. While form components with internal state may re-render frequently anyway, memoizing can still help when parent components re-render without changing this component's props.Suggested approach
-export const WorkflowRunForm = ({ +export const WorkflowRunForm = React.memo(({ workflowVariables, // ... other props }: WorkflowRunFormProps) => { // ... component body -}; +});
100-115: Unused prop andanytype usage.
Line 107:
pollingErroris declared in the interface but never used in the component. Remove it if not needed.Line 113:
workflowAppis typed asany. Per coding guidelines, avoidanyand use proper types orunknownwith type guards.Suggested fix
interface WorkflowRunFormProps { workflowVariables: WorkflowVariable[]; onSubmitVariables: (variables: WorkflowVariable[]) => Promise<void>; loading: boolean; executionId?: string | null; workflowStatus?: WorkflowExecutionStatus | null; isPolling?: boolean; - pollingError?: any; isRunning?: boolean; onRunningChange?: (isRunning: boolean) => void; canvasId?: string; className?: string; templateContent?: string; - workflowApp?: any; + workflowApp?: { + canvasData?: RawCanvasData; + // Add other known properties as needed + }; creditUsage?: number | null; }
| const isFormValid = useMemo(() => { | ||
| return workflowVariables.every((variable) => { | ||
| if (!variable.required) { | ||
| return true; | ||
| } | ||
|
|
||
| const value = variableValues[variable.name]; | ||
|
|
||
| if (variable.variableType === 'string') { | ||
| return value && value.trim() !== ''; | ||
| } | ||
|
|
||
| if (variable.variableType === 'option') { | ||
| return value && (Array.isArray(value) ? value.length > 0 : value); | ||
| } | ||
|
|
||
| if (variable.variableType === 'resource') { | ||
| return value && Array.isArray(value) && value.length > 0; | ||
| } | ||
|
|
||
| return false; | ||
| }); | ||
| }, [workflowVariables, variableValues]); |
There was a problem hiding this comment.
Potential runtime error and code duplication.
-
Line 231: If
valueis truthy but not a string (e.g., an object), callingvalue.trim()will throw a runtime error. Add a type check. -
Line 242: Returning
falsefor unknown variable types means any new type will be treated as invalid. Consider returningtruefor unhandled types or logging a warning. -
The validation logic is duplicated in
getInvalidFields(lines 247-268). Consider extracting a shared validation helper.
Suggested fix for type safety
if (variable.variableType === 'string') {
- return value && value.trim() !== '';
+ return typeof value === 'string' && value.trim() !== '';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isFormValid = useMemo(() => { | |
| return workflowVariables.every((variable) => { | |
| if (!variable.required) { | |
| return true; | |
| } | |
| const value = variableValues[variable.name]; | |
| if (variable.variableType === 'string') { | |
| return value && value.trim() !== ''; | |
| } | |
| if (variable.variableType === 'option') { | |
| return value && (Array.isArray(value) ? value.length > 0 : value); | |
| } | |
| if (variable.variableType === 'resource') { | |
| return value && Array.isArray(value) && value.length > 0; | |
| } | |
| return false; | |
| }); | |
| }, [workflowVariables, variableValues]); | |
| const isFormValid = useMemo(() => { | |
| return workflowVariables.every((variable) => { | |
| if (!variable.required) { | |
| return true; | |
| } | |
| const value = variableValues[variable.name]; | |
| if (variable.variableType === 'string') { | |
| return typeof value === 'string' && value.trim() !== ''; | |
| } | |
| if (variable.variableType === 'option') { | |
| return value && (Array.isArray(value) ? value.length > 0 : value); | |
| } | |
| if (variable.variableType === 'resource') { | |
| return value && Array.isArray(value) && value.length > 0; | |
| } | |
| return false; | |
| }); | |
| }, [workflowVariables, variableValues]); |
🤖 Prompt for AI Agents
In
`@packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx`
around lines 222 - 244, The current validation in isFormValid can throw if value
isn't a string and treats unknown variable types as invalid while duplicating
logic in getInvalidFields; extract the per-variable validation into a shared
helper (e.g., validateVariable(variable, variableValues)) and use it from both
isFormValid and getInvalidFields, ensure the helper checks typeof value ===
'string' before calling trim for variable.variableType === 'string', handle
'option' and 'resource' as before, and for unrecognized variable.variableType
either return true or call a logger/warn then return true so new types don't
break validation.
| const handleFileUpload = useCallback( | ||
| async (file: File, variableName: string) => { | ||
| const currentFileList = variableValues[variableName] || []; | ||
| const result = await uploadFile(file, currentFileList); | ||
|
|
||
| if (result && typeof result === 'object' && 'storageKey' in result) { | ||
| let fileId: string | undefined; | ||
|
|
||
| // Create DriveFile if canvasId is available | ||
| if (canvasId) { | ||
| const variable = workflowVariables.find((v) => v.name === variableName); | ||
| if (variable?.variableId) { | ||
| try { | ||
| const { data: driveFileResponse, error } = await getClient().createDriveFile({ | ||
| body: { | ||
| canvasId, | ||
| name: file.name, | ||
| type: file.type, | ||
| storageKey: result.storageKey, | ||
| source: 'variable', | ||
| variableId: variable.variableId, | ||
| }, | ||
| }); | ||
|
|
||
| if (!error && driveFileResponse?.data?.fileId) { | ||
| fileId = driveFileResponse.data.fileId; | ||
| } | ||
| } catch (error) { | ||
| console.error('Failed to create DriveFile:', error); | ||
| // Continue without fileId if creation fails | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Create new file with storageKey and optional fileId | ||
| const newFile: UploadFile = { | ||
| uid: fileId || result.uid, | ||
| name: file.name, | ||
| status: 'done', | ||
| url: result.storageKey, // Store storageKey in url field | ||
| ...(fileId && { response: { fileId } }), // Store fileId in response for later retrieval | ||
| }; | ||
|
|
||
| // Replace the file list with the new file (single file limit) | ||
| const newFileList = [newFile]; | ||
| handleValueChange(variableName, newFileList); | ||
| form.setFieldsValue({ | ||
| [variableName]: newFileList, | ||
| }); | ||
| return false; // Prevent default upload behavior | ||
| } | ||
| return false; | ||
| }, | ||
| [uploadFile, variableValues, canvasId, workflowVariables], | ||
| ); |
There was a problem hiding this comment.
Missing handleValueChange in dependency array and form reference.
The callback uses handleValueChange (line 424) and form (line 425) but neither is included in the dependency array. Once handleValueChange is wrapped with useCallback, add it to the dependencies. The form instance from Form.useForm() is stable, but it's good practice to include it.
Suggested fix
- [uploadFile, variableValues, canvasId, workflowVariables],
+ [uploadFile, variableValues, canvasId, workflowVariables, handleValueChange, form],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleFileUpload = useCallback( | |
| async (file: File, variableName: string) => { | |
| const currentFileList = variableValues[variableName] || []; | |
| const result = await uploadFile(file, currentFileList); | |
| if (result && typeof result === 'object' && 'storageKey' in result) { | |
| let fileId: string | undefined; | |
| // Create DriveFile if canvasId is available | |
| if (canvasId) { | |
| const variable = workflowVariables.find((v) => v.name === variableName); | |
| if (variable?.variableId) { | |
| try { | |
| const { data: driveFileResponse, error } = await getClient().createDriveFile({ | |
| body: { | |
| canvasId, | |
| name: file.name, | |
| type: file.type, | |
| storageKey: result.storageKey, | |
| source: 'variable', | |
| variableId: variable.variableId, | |
| }, | |
| }); | |
| if (!error && driveFileResponse?.data?.fileId) { | |
| fileId = driveFileResponse.data.fileId; | |
| } | |
| } catch (error) { | |
| console.error('Failed to create DriveFile:', error); | |
| // Continue without fileId if creation fails | |
| } | |
| } | |
| } | |
| // Create new file with storageKey and optional fileId | |
| const newFile: UploadFile = { | |
| uid: fileId || result.uid, | |
| name: file.name, | |
| status: 'done', | |
| url: result.storageKey, // Store storageKey in url field | |
| ...(fileId && { response: { fileId } }), // Store fileId in response for later retrieval | |
| }; | |
| // Replace the file list with the new file (single file limit) | |
| const newFileList = [newFile]; | |
| handleValueChange(variableName, newFileList); | |
| form.setFieldsValue({ | |
| [variableName]: newFileList, | |
| }); | |
| return false; // Prevent default upload behavior | |
| } | |
| return false; | |
| }, | |
| [uploadFile, variableValues, canvasId, workflowVariables], | |
| ); | |
| const handleFileUpload = useCallback( | |
| async (file: File, variableName: string) => { | |
| const currentFileList = variableValues[variableName] || []; | |
| const result = await uploadFile(file, currentFileList); | |
| if (result && typeof result === 'object' && 'storageKey' in result) { | |
| let fileId: string | undefined; | |
| // Create DriveFile if canvasId is available | |
| if (canvasId) { | |
| const variable = workflowVariables.find((v) => v.name === variableName); | |
| if (variable?.variableId) { | |
| try { | |
| const { data: driveFileResponse, error } = await getClient().createDriveFile({ | |
| body: { | |
| canvasId, | |
| name: file.name, | |
| type: file.type, | |
| storageKey: result.storageKey, | |
| source: 'variable', | |
| variableId: variable.variableId, | |
| }, | |
| }); | |
| if (!error && driveFileResponse?.data?.fileId) { | |
| fileId = driveFileResponse.data.fileId; | |
| } | |
| } catch (error) { | |
| console.error('Failed to create DriveFile:', error); | |
| // Continue without fileId if creation fails | |
| } | |
| } | |
| } | |
| // Create new file with storageKey and optional fileId | |
| const newFile: UploadFile = { | |
| uid: fileId || result.uid, | |
| name: file.name, | |
| status: 'done', | |
| url: result.storageKey, // Store storageKey in url field | |
| ...(fileId && { response: { fileId } }), // Store fileId in response for later retrieval | |
| }; | |
| // Replace the file list with the new file (single file limit) | |
| const newFileList = [newFile]; | |
| handleValueChange(variableName, newFileList); | |
| form.setFieldsValue({ | |
| [variableName]: newFileList, | |
| }); | |
| return false; // Prevent default upload behavior | |
| } | |
| return false; | |
| }, | |
| [uploadFile, variableValues, canvasId, workflowVariables, handleValueChange, form], | |
| ); |
🤖 Prompt for AI Agents
In
`@packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx`
around lines 379 - 433, The handleFileUpload useCallback references
handleValueChange and form but they are missing from its dependency array;
update the dependency array for handleFileUpload to include handleValueChange
and form (in addition to uploadFile, variableValues, canvasId,
workflowVariables) so React's hook rules are satisfied—ensure handleValueChange
is memoized with useCallback where it's defined before adding it, and then add
both handleValueChange and form to the dependency list of handleFileUpload.
- Removed unused state variables for tools panel and highlight install buttons. - Eliminated the handleToolsDependencyOpenChange function to streamline the component. - Ensured consistent use of optional chaining and nullish coalescing for safer property access.
…unForm - Eliminated the _handleTemplateVariableChange function to streamline the component. - Ensured consistent use of Tailwind CSS for styling and maintained performance optimizations.
- Eliminated the RequiredTagText component as it returned null, streamlining the code. - Ensured consistent use of Tailwind CSS for styling and maintained performance optimizations.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx`:
- Around line 369-375: The handleValueChange function is recreated each render
causing needless re-renders in child components (e.g., Input.TextArea, Select)
and unstable dependencies; wrap handleValueChange in React.useCallback and
return the same updater signature, referencing setVariableValues in the
dependency array (e.g., useCallback((variableName, value) => {
setVariableValues(prev => ({ ...prev, [variableName]: value })); },
[setVariableValues])) so child props and other callbacks receive a stable
function reference.
- Around line 435-445: The handleFileRemove callback uses the `form` object
(calls `form.setFieldsValue`) but `form` is not included in the dependency
array, which can cause stale closures; update the dependency array for
`useCallback` on `handleFileRemove` to include `form` (i.e., [variableValues,
handleValueChange, form]) so the callback is recreated when the form instance
changes, and verify no other missing dependencies exist for `handleFileRemove`.
♻️ Duplicate comments (2)
packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx (2)
221-275: Type safety and code duplication issues persist.The previously flagged issues remain:
Line 230:
value.trim()is called without verifyingvalueis a string. Ifvalueis an object, this throws a runtime error.Line 241: Returns
falsefor unknown variable types, treating new types as invalid.Lines 254-260: Validation logic duplicates what's in
isFormValid. Extract a shared helper.Suggested refactor: extract shared validation helper
+// Shared validation helper for single variable +const isVariableValid = (variable: WorkflowVariable, value: unknown): boolean => { + if (!variable.required) return true; + + if (variable.variableType === 'string') { + return typeof value === 'string' && value.trim() !== ''; + } + if (variable.variableType === 'option') { + return value != null && (Array.isArray(value) ? value.length > 0 : !!value); + } + if (variable.variableType === 'resource') { + return Array.isArray(value) && value.length > 0; + } + // Unknown types: return true to avoid blocking new types + return true; +}; const isFormValid = useMemo(() => { - return workflowVariables.every((variable) => { - if (!variable.required) { - return true; - } - - const value = variableValues[variable.name]; - - if (variable.variableType === 'string') { - return value && value.trim() !== ''; - } - // ... rest of logic - return false; - }); + return workflowVariables.every((variable) => + isVariableValid(variable, variableValues[variable.name]) + ); }, [workflowVariables, variableValues]); const getInvalidFields = useCallback(() => { const invalidFields: string[] = []; for (const variable of workflowVariables) { - if (!variable.required) continue; - const value = variableValues[variable.name]; - let isValid = false; - // ... duplicated validation logic + if (!isVariableValid(variable, variableValues[variable.name])) { + invalidFields.push(variable.name); + } } return invalidFields; }, [workflowVariables, variableValues]);
378-432: Missing dependencies inuseCallback.
handleValueChange(line 423) andform(lines 424-426) are used inside this callback but are not in the dependency array. OncehandleValueChangeis wrapped withuseCallback, add both to the dependencies.Suggested fix
- [uploadFile, variableValues, canvasId, workflowVariables], + [uploadFile, variableValues, canvasId, workflowVariables, handleValueChange, form],
🧹 Nitpick comments (7)
packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx (7)
57-64: Wrap pure component withReact.memo.Per coding guidelines, pure presentational components should be wrapped with
React.memoto prevent unnecessary re-renders.Suggested fix
-const RequiredTagText = () => { +const RequiredTagText = React.memo(() => { const { t } = useTranslation(); return ( <div className="flex-shrink-0 text-[10px] text-refly-text-2 leading-[16px] px-1 border-[1px] border-solid border-refly-Card-Border rounded-[4px]"> {t('canvas.workflow.variables.required') || 'Required'} </div> ); -}; +}); +RequiredTagText.displayName = 'RequiredTagText';
84-97: Wrap pure component withReact.memo.
FormItemLabelis a pure presentational component and should be memoized to prevent unnecessary re-renders when parent state changes.Suggested fix
-const FormItemLabel = ({ name, required }: { name: string; required: boolean }) => { +const FormItemLabel = React.memo(({ name, required }: { name: string; required: boolean }) => { return ( <div className="flex items-center gap-2 min-w-0"> <Typography.Paragraph ellipsis={{ rows: 1, tooltip: <div className="max-h-[200px] overflow-y-auto">{name}</div> }} className="!m-0 text-xs font-semibold text-refly-text-0 leading-4 max-w-[100px]" > {name} </Typography.Paragraph> {required && <RequiredTagText />} </div> ); -}; +}); +FormItemLabel.displayName = 'FormItemLabel';
497-502: Consider addingformto dependency array.
form.setFieldsValueis called butformis not in the dependencies. While theforminstance fromForm.useForm()is stable, including it follows React's exhaustive-deps best practice.Suggested fix
-}, [workflowVariables, convertVariableToFormValue]); +}, [workflowVariables, convertVariableToFormValue, form]);
691-805: Consider extractingrenderFormFieldinto a memoized component.This render function is recreated on every render and captures multiple state variables. Per coding guidelines, nested components with closures should be split into separate components. Extracting this to a
FormFieldcomponent with props would improve performance and maintainability.Outline of suggested refactor
// Extract to a separate memoized component interface FormFieldProps { variable: WorkflowVariable; value: any; hasError: boolean; isDisabled: boolean; uploading: boolean; onValueChange: (name: string, value: any) => void; onFileUpload: (file: File, name: string) => Promise<boolean>; onFileRemove: (file: UploadFile, name: string) => void; onRefresh: (name: string) => void; } const FormField = React.memo(({ variable, value, hasError, ... }: FormFieldProps) => { const { t } = useTranslation(); // ... render logic based on variable.variableType }); // Then in the parent: {workflowVariables.map((variable) => ( <FormField key={variable.name} variable={variable} value={variableValues[variable.name]} hasError={isFieldInvalid(variable.name)} isDisabled={isFormDisabled} uploading={uploading} onValueChange={handleValueChange} onFileUpload={handleFileUpload} onFileRemove={handleFileRemove} onRefresh={handleRefreshFile} /> ))}
629-680: Consider typing the catch block error.The error variable is implicitly
any. Adding a type guard or explicit typing would improve safety:Suggested improvement
} catch (error) { + // Type guard for Ant Design form validation error + const formError = error as { errorFields?: Array<{ name: (string | number)[] }> }; // Form validation failed, scroll to first error - if (error?.errorFields && error.errorFields.length > 0) { - const firstErrorField = error.errorFields[0]; + if (formError?.errorFields && formError.errorFields.length > 0) { + const firstErrorField = formError.errorFields[0];
818-821: Remove unnecessary constant-condition wrapper.The
biome-ignorecomment and the always-true condition suggest leftover code from a refactor. The wrapper can be removed to simplify the JSX structure.Suggested simplification
<> - {/* default show Form */} - {/* biome-ignore lint/correctness/noConstantCondition: <explanation> */} - { - <div className="p-3 sm:p-4 flex-1 overflow-y-auto"> + <div className="p-3 sm:p-4 flex-1 overflow-y-auto"> {/* Show loading state when loading */} {workflowVariables.length > 0 ? ( ... )} - </div> - } + </div>
702-731: Verify label-input accessibility association.Using a custom
FormItemLabelcomponent as thelabelprop may not automatically associate the label with the input for screen readers. Ant Design'sForm.Itemexpects a string or ReactNode, but thehtmlForassociation may not work properly with a custom component.Consider verifying that the label is properly associated with the input, or add explicit
aria-labelledbyattributes. Based on learnings, ensure proper labeling and accessibility for form elements.
| // Handle form value changes | ||
| const handleValueChange = (variableName: string, value: any) => { | ||
| setVariableValues((prev) => ({ | ||
| ...prev, | ||
| [variableName]: value, | ||
| })); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Wrap handleValueChange with useCallback.
This function is passed to child components (e.g., Input.TextArea, Select) and used in other callbacks' dependency arrays. Without useCallback, a new function reference is created on every render, causing unnecessary re-renders.
Suggested fix
-const handleValueChange = (variableName: string, value: any) => {
+const handleValueChange = useCallback((variableName: string, value: any) => {
setVariableValues((prev) => ({
...prev,
[variableName]: value,
}));
-};
+}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle form value changes | |
| const handleValueChange = (variableName: string, value: any) => { | |
| setVariableValues((prev) => ({ | |
| ...prev, | |
| [variableName]: value, | |
| })); | |
| }; | |
| // Handle form value changes | |
| const handleValueChange = useCallback((variableName: string, value: any) => { | |
| setVariableValues((prev) => ({ | |
| ...prev, | |
| [variableName]: value, | |
| })); | |
| }, []); |
🤖 Prompt for AI Agents
In
`@packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx`
around lines 369 - 375, The handleValueChange function is recreated each render
causing needless re-renders in child components (e.g., Input.TextArea, Select)
and unstable dependencies; wrap handleValueChange in React.useCallback and
return the same updater signature, referencing setVariableValues in the
dependency array (e.g., useCallback((variableName, value) => {
setVariableValues(prev => ({ ...prev, [variableName]: value })); },
[setVariableValues])) so child props and other callbacks receive a stable
function reference.
| const handleFileRemove = useCallback( | ||
| (file: UploadFile, variableName: string) => { | ||
| const currentFileList = variableValues[variableName] || []; | ||
| const newFileList = currentFileList.filter((f: UploadFile) => f.uid !== file.uid); | ||
| handleValueChange(variableName, newFileList); | ||
| form.setFieldsValue({ | ||
| [variableName]: newFileList, | ||
| }); | ||
| }, | ||
| [variableValues, handleValueChange], | ||
| ); |
There was a problem hiding this comment.
Missing form in dependency array.
The callback uses form.setFieldsValue but form is not included in the dependency array.
Suggested fix
- [variableValues, handleValueChange],
+ [variableValues, handleValueChange, form],🤖 Prompt for AI Agents
In
`@packages/ai-workspace-common/src/components/canvas/workflow-run/workflow-run-form.tsx`
around lines 435 - 445, The handleFileRemove callback uses the `form` object
(calls `form.setFieldsValue`) but `form` is not included in the dependency
array, which can cause stale closures; update the dependency array for
`useCallback` on `handleFileRemove` to include `form` (i.e., [variableValues,
handleValueChange, form]) so the callback is recreated when the form instance
changes, and verify no other missing dependencies exist for `handleFileRemove`.
mrcfps
changed the title
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.