fix: workflow step validation #3965
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Kiryous
changed the title
# Conflicts: # keep-ui/package-lock.json # keep-ui/package.json
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR implements comprehensive YAML validation for workflow steps to properly handle misplaced configuration keys like enrich_alert.
- Added new
validate-yaml.tswith position-aware error reporting using Zod schemas to catch misplaced keys in workflow definitions - Updated workflow validation to return arrays of errors with severity levels instead of single strings in
validation.ts - Added
getCurrentPathfunction inyaml-utils.tsto accurately identify YAML node locations for error reporting - Added
useWorkflowJsonSchemahook and schema validation using Monaco editor integration for real-time validation feedback - Improved test coverage with new validation test cases in
validateWorkflowYaml.test.tsandparser.test.ts
💡 (4/5) You can add custom instructions or style guidelines for the bot here!
29 file(s) reviewed, 21 comment(s)
Edit PR Review Bot Settings | Greptile
| data: response, | ||
| filename: "topology-export.yaml", | ||
| contentType: "application/x-yaml", | ||
| }); | ||
| } catch (error) { | ||
| console.log(error); |
There was a problem hiding this comment.
style: Consider using toast.error here instead of console.log for consistency with error handling elsewhere in the file
| const path = getCurrentPath(doc, 69); | ||
| expect(path).toEqual(["workflow", "steps", 0, "provider", "type"]); |
There was a problem hiding this comment.
style: Magic number 69 is used without explanation - unclear why this specific position maps to type field. Should test with actual character position calculation.
| <Card className="h-[calc(100vh-12rem)] p-0 overflow-hidden"> | ||
| <MonacoYAMLEditor | ||
| <Card className="h-[calc(100vh-12rem)] p-0"> | ||
| <YAMLEditor | ||
| key={workflow.workflow_raw!} |
There was a problem hiding this comment.
logic: key prop using non-null assertion (!), but workflow.workflow_raw could be undefined based on type
| key={workflow.workflow_raw!} | |
| key={workflow.workflow_raw ?? ''} |
| <Card className="h-[calc(100vh-12rem)] p-0 overflow-hidden"> | ||
| <MonacoYAMLEditor | ||
| <Card className="h-[calc(100vh-12rem)] p-0"> | ||
| <YAMLEditor | ||
| key={workflow.workflow_raw!} | ||
| workflowRaw={workflow.workflow_raw!} |
There was a problem hiding this comment.
logic: same non-null assertion issue with workflowRaw prop
| workflowRaw={workflow.workflow_raw!} | |
| workflowRaw={workflow.workflow_raw ?? ''} |
| expect(result).toEqual([["No test provider selected", "warning"]]); | ||
| }); |
There was a problem hiding this comment.
logic: Error message format changed from string to tuple with warning type. Ensure all error handlers in the codebase are updated to handle this new format.
| result.forEach(([key, error]) => { | ||
| validationErrors.push([key, error, "error"]); | ||
| }); | ||
| isValid = result.length === 0; |
There was a problem hiding this comment.
logic: isValid logic is incorrect - should be false if any errors exist, not just if global validation has errors
| isValid = result.length === 0; | |
| isValid = false; |
| [...step.branches.true, ...step.branches.false].forEach((branch) => { | ||
| const errors = validateStepPure( | ||
| branch, | ||
| providers, | ||
| installedProviders, | ||
| definition | ||
| ); | ||
| if (errors.length > 0) { | ||
| errors.forEach(([error, type]) => { | ||
| validationErrors.push([branch.name || branch.id, error, type]); | ||
| }); | ||
| isValid = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
logic: errors variable is being shadowed inside the forEach loop - could lead to confusion and bugs
| loader.config({ monaco }); | ||
|
|
||
| // In the docs, it is stated that there should only be one monaco yaml instance configured at a time | ||
| let monacoYamlInstance: MonacoYaml | undefined; |
There was a problem hiding this comment.
logic: Global mutable state could cause issues in development with React strict mode's double-mounting. Consider using React context or refs instead.
Closes #3825