-
Notifications
You must be signed in to change notification settings - Fork 798
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
Validate json and don't allow null parameter values when running workflow #866
Conversation
…src/' <!-- ELLIPSIS_HIDDEN --> | 🚀 | This description was created by 31e6dddd6a9efbec766b7f08e652e4fcaca36a04 | |--------|--------| fix: add null check validation for required workflow parameters and update import paths ### Summary: Added null check validation for required fields in `RunWorkflowForm` and updated import paths for `WorkflowParameter`. **Key points**: - **Validation**: - Added null check validation in `RunWorkflowForm` to ensure required fields are not empty. - **Imports**: - Moved `WorkflowParameter` import from `@/api/types` to `./types/workflowTypes` in `RunWorkflowForm.tsx` and `WorkflowRunParameters.tsx`. ---- Generated with ❤️ by [ellipsis.dev](https://www.ellipsis.dev) <!-- ELLIPSIS_HIDDEN -->
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.
👍 Looks good to me! Reviewed everything up to a5e4c80 in 25 seconds
More details
- Looked at
47
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:155
- Draft comment:
Consider checking forundefined
as well to ensure all required fields are validated properly.
if (value === null || value === undefined) {
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is about a change made in the diff, specifically the validation logic for form fields. Checking for 'undefined' could be relevant if the form field can be 'undefined', which is common in JavaScript. The suggestion is actionable and clear, as it directly relates to form validation logic.
The current code might already handle 'undefined' values elsewhere, or 'undefined' might not be a valid state for these fields. Without more context, it's hard to be certain if the change is necessary.
Given that 'undefined' is a common state for uninitialized variables in JavaScript, the suggestion to check for it in validation logic is reasonable and likely beneficial.
Keep the comment as it suggests a reasonable improvement to the validation logic by checking for 'undefined', which is a common scenario in JavaScript.
Workflow ID: wflow_HH5BbXsm4fgiQHBv
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Incremental review on a5e4c80 in 31 seconds
More details
- Looked at
47
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/RunWorkflowForm.tsx:155
- Draft comment:
Consider checking for empty strings as well, as they can also represent missing values in forms.
if (value === null || value === '') {
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is relevant to the change made in the diff, which added a validation rule for null values. Checking for empty strings is a common practice in form validation to ensure that fields are not left blank. This suggestion seems actionable and could improve the robustness of the validation logic.
The current validation logic might already handle empty strings elsewhere, or the application might have specific requirements that allow empty strings. Without full context, it's hard to be certain that this change is necessary.
Given the context provided, it seems reasonable to assume that checking for empty strings would be a beneficial addition to the validation logic, unless there is a specific reason not to do so.
The comment is relevant and suggests an actionable improvement to the validation logic. It should be kept as it addresses a potential issue with form validation.
Workflow ID: wflow_UcOSnR9Vdt3A5CXi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
fix: add null check validation for workflow parameters and update import paths
Summary:
Add null check validation for required workflow parameters and update import paths in
RunWorkflowForm.tsx
andWorkflowRunParameters.tsx
.Key points:
RunWorkflowForm
to ensure required fields are not null.WorkflowParameter
import path from@/api/types
to./types/workflowTypes
inRunWorkflowForm.tsx
andWorkflowRunParameters.tsx
.Generated with ❤️ by ellipsis.dev