Skip to content
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

Improve wait time input #1681

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Improve wait time input #1681

merged 1 commit into from
Jan 30, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Jan 30, 2025

Important

Improves wait time input handling in WaitNode by changing input type to string and adding numeric validation.

  • Behavior:
    • Replaces WorkflowBlockInput with Input in WaitNode.tsx for wait time input.
    • Changes waitInSeconds type from number to string in types.ts.
    • Adds validation for numeric input in getWorkflowErrors() in workflowEditorUtils.ts.
  • Data Handling:
    • Converts waitInSeconds to string in convertToNode() in workflowEditorUtils.ts.
    • Parses waitInSeconds back to number in getWorkflowBlock() in workflowEditorUtils.ts.
  • Misc:
    • Imports isWaitNode in workflowEditorUtils.ts.

This description was created by Ellipsis for 45b1c98. It will automatically update as commits are pushed.

…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Improves wait time input handling in `WaitNode` by using a standard input component, changing data type to string, and adding validation.
>
>   - **Behavior**:
>     - Replaces `WorkflowBlockInput` with `Input` in `WaitNode.tsx` for wait time input.
>     - Changes `waitInSeconds` type from `number` to `string` in `WaitNodeData`.
>     - Adds validation for `waitInSeconds` to ensure it is a numeric string in `getWorkflowErrors()`.
>   - **Data Handling**:
>     - Converts `waitInSeconds` to string in `convertToNode()` and back to number in `getWorkflowBlock()`.
>   - **Imports**:
>     - Adds `isWaitNode` import in `workflowEditorUtils.ts`.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 8ac2a6377ed74165647b56a4984530efdf833162. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Incremental review on 45b1c98 in 56 seconds

More details
  • Looked at 108 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/editor/nodes/WaitNode/types.ts:5
  • Draft comment:
    Consider converting waitInSeconds to a number when used in calculations or logic to avoid potential type issues.
  • Reason this comment was not posted:
    Marked as duplicate.
2. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:965
  • Draft comment:
    Ensure waitInSeconds is consistently converted to a number when used in logic or calculations.
        wait_sec: parseInt(node.data.waitInSeconds, 10),
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_HMhyoNp20r5rCkt6


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 45b1c98 in 1 minute and 44 seconds

More details
  • Looked at 108 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/editor/nodes/WaitNode/types.ts:5
  • Draft comment:
    Ensure waitInSeconds is consistently converted to a number where necessary, such as when using it in calculations or logic that requires a number.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:965
  • Draft comment:
    Ensure waitInSeconds is converted to a number wherever it is used in a numeric context, not just in getWorkflowBlock.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_Q0OC9dkX7NzODp8E


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@msalihaltun msalihaltun merged commit 4c50f6f into main Jan 30, 2025
7 checks passed
@msalihaltun msalihaltun deleted the salih/improve-wait-time-input branch January 30, 2025 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants