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

Add navigation block #1256

Merged
merged 3 commits into from
Nov 25, 2024
Merged

Add navigation block #1256

merged 3 commits into from
Nov 25, 2024

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Nov 25, 2024

Important

Add NavigationNode component for navigation tasks in workflows, integrating it into the workflow editor and YAML serialization.

  • Behavior:
    • Add NavigationNode component in NavigationNode.tsx for navigation tasks in workflows.
    • Introduce NavigationNodeData and NavigationNode types in NavigationNode/types.ts.
    • Add NavigationBlock type in workflowTypes.ts and workflowYamlTypes.ts.
  • Integration:
    • Update nodeTypes in index.ts to include NavigationNode.
    • Add NavigationNode to WorkflowNodeLibraryPanel.tsx for user selection.
    • Modify workflowEditorUtils.ts to handle NavigationNode in node creation and YAML conversion.
  • Refactoring:
    • Move common tooltip content and placeholders to constants.ts for reuse.
    • Remove redundant helpTooltipContent and fieldPlaceholders from ActionNode/types.ts.
  • Misc:
    • Add RobotIcon in RobotIcon.tsx for NavigationNode representation.

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

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add `NavigationNode` component to handle navigation tasks in workflows, integrating it into the workflow editor and YAML serialization.
>
>   - **New Features**:
>     - Add `NavigationNode` component in `NavigationNode.tsx` to handle navigation tasks in workflows.
>     - Introduce `NavigationNodeData` and `NavigationNode` types in `NavigationNode/types.ts`.
>     - Add `NavigationBlock` type in `workflowTypes.ts` and `workflowYamlTypes.ts`.
>   - **Integration**:
>     - Update `nodeTypes` in `index.ts` to include `NavigationNode`.
>     - Add `NavigationNode` to `WorkflowNodeLibraryPanel.tsx` for user selection.
>     - Modify `workflowEditorUtils.ts` to handle `NavigationNode` in node creation and YAML conversion.
>   - **Refactoring**:
>     - Move common tooltip content and placeholders to `constants.ts` for reuse.
>     - Remove redundant `helpTooltipContent` and `fieldPlaceholders` from `ActionNode/types.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 41bb7fbaa5d94e484642dbbc4434b7bc08100931. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
…src/'

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add `NavigationNode` component to handle navigation tasks in workflows, integrating it into the workflow editor and YAML serialization.
>
>   - **New Features**:
>     - Add `NavigationNode` component in `NavigationNode.tsx` to handle navigation tasks in workflows.
>     - Introduce `NavigationNodeData` and `NavigationNode` types in `NavigationNode/types.ts`.
>     - Add `NavigationBlock` type in `workflowTypes.ts` and `workflowYamlTypes.ts`.
>   - **Integration**:
>     - Update `nodeTypes` in `index.ts` to include `NavigationNode`.
>     - Add `NavigationNode` to `WorkflowNodeLibraryPanel.tsx` for user selection.
>     - Modify `workflowEditorUtils.ts` to handle `NavigationNode` in node creation and YAML conversion.
>   - **Refactoring**:
>     - Move common tooltip content and placeholders to `constants.ts` for reuse.
>     - Remove redundant `helpTooltipContent` and `fieldPlaceholders` from `ActionNode/types.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 41bb7fbaa5d94e484642dbbc4434b7bc08100931. 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.

👍 Looks good to me! Incremental review on 3d54048 in 40 seconds

More details
  • Looked at 1023 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. skyvern-frontend/src/components/icons/RobotIcon.tsx:17
  • Draft comment:
    Consider making the fill color customizable by passing it as a prop to make the component more reusable.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The RobotIcon component uses a hardcoded fill color #F8FAFC. This could be made customizable by passing it as a prop, which would make the component more reusable.
2. skyvern-frontend/src/routes/workflows/editor/constants.ts:40
  • Draft comment:
    The navigationGoal placeholder seems specific to ActionNode and is not used in NavigationNode. Consider moving it to where it's used to avoid confusion.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The commonFieldPlaceholders object has a placeholder for navigationGoal that is not used in the NavigationNode component. It seems like a placeholder specific to ActionNode. This could lead to confusion or redundancy.
3. skyvern-frontend/src/routes/workflows/editor/nodes/ActionNode/ActionNode.tsx:32
  • Draft comment:
    Consider using commonFieldPlaceholders.navigationGoal instead of the local navigationGoalPlaceholder to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The ActionNode component has a local constant navigationGoalPlaceholder which is the same as the one in commonFieldPlaceholders. This redundancy can be removed by using the common constant.
4. skyvern-frontend/src/routes/workflows/editor/nodes/ActionNode/ActionNode.tsx:112
  • Draft comment:
    Use commonFieldPlaceholders.navigationGoal instead of the local navigationGoalPlaceholder to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The ActionNode component imports commonFieldPlaceholders but does not use the navigationGoal placeholder from it, leading to redundancy.
5. skyvern-frontend/src/routes/workflows/editor/nodes/NavigationNode/NavigationNode.tsx:31
  • Draft comment:
    Consider using commonFieldPlaceholders.url instead of the local urlPlaceholder to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The NavigationNode component has a local constant urlPlaceholder which is the same as the one in commonFieldPlaceholders. This redundancy can be removed by using the common constant.
6. skyvern-frontend/src/routes/workflows/editor/nodes/NavigationNode/NavigationNode.tsx:34
  • Draft comment:
    Consider using commonFieldPlaceholders.navigationGoal instead of the local navigationGoalPlaceholder to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The NavigationNode component has a local constant navigationGoalPlaceholder which is similar to the one in commonFieldPlaceholders. This redundancy can be removed by using the common constant.
7. skyvern-frontend/src/routes/workflows/editor/nodes/NavigationNode/NavigationNode.tsx:117
  • Draft comment:
    Use commonFieldPlaceholders.url instead of the local urlPlaceholder to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The NavigationNode component imports commonFieldPlaceholders but does not use the url placeholder from it, leading to redundancy.
8. skyvern-frontend/src/routes/workflows/editor/nodes/NavigationNode/NavigationNode.tsx:134
  • Draft comment:
    Use commonFieldPlaceholders.navigationGoal instead of the local navigationGoalPlaceholder to avoid redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The NavigationNode component imports commonFieldPlaceholders but does not use the navigationGoal placeholder from it, leading to redundancy.

Workflow ID: wflow_hExxGUzmYu7j346c


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.

👍 Looks good to me! Reviewed everything up to 3d54048 in 2 minutes and 30 seconds

More details
  • Looked at 1241 lines of code in 13 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern/forge/agent_functions.py:99
  • Draft comment:
    The function _convert_svg_to_string is defined twice in this file. Consider removing the duplicate to avoid redundancy.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. skyvern/forge/agent_functions.py:172
  • Draft comment:
    The function _convert_css_shape_to_string is defined twice in this file. Consider removing the duplicate to avoid redundancy.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. skyvern-frontend/src/routes/workflows/editor/constants.ts:38
  • Draft comment:
    Consider using the existing fieldPlaceholders constant instead of creating a new one.

  • constant fieldPlaceholders (types.ts)

  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is suggesting a refactor to avoid redundancy by using an existing constant. However, the differences in the constants suggest that they might serve different purposes. Without more context on how these constants are used, it's difficult to determine if the suggestion is valid. The comment might be speculative if the new constant is intended for a different use case.
    The comment assumes that the two constants can be merged without understanding their specific use cases. The differences in the constants might be intentional and necessary for different contexts.
    The differences in the constants suggest that they might be used in different contexts, which could justify having separate constants. Without more context, it's safer to assume the new constant serves a specific purpose.
    The comment should be deleted as it assumes the constants can be merged without understanding their specific use cases, which might not be the case.

Workflow ID: wflow_y51JIdqQX1Le2yDl


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.

👍 Looks good to me! Incremental review on a5bca43 in 29 seconds

More details
  • Looked at 217 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. skyvern/forge/agent.py:624
  • Draft comment:
    The 'organization' parameter was removed from the 'agent_step' function signature but was not used in the function body. This change is appropriate as it simplifies the function signature without affecting functionality.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The removal of the 'organization' parameter from several functions in 'agent.py' and 'agent_functions.py' seems intentional, as the parameter was not being used in the function bodies. This change simplifies the function signatures and reduces unnecessary complexity.
2. skyvern/forge/agent.py:1098
  • Draft comment:
    The 'organization' parameter was removed from the '_scrape_with_type' function signature but was not used in the function body. This change is appropriate as it simplifies the function signature without affecting functionality.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The removal of the 'organization' parameter from several functions in 'agent.py' and 'agent_functions.py' seems intentional, as the parameter was not being used in the function bodies. This change simplifies the function signatures and reduces unnecessary complexity.
3. skyvern/forge/agent.py:1130
  • Draft comment:
    The 'organization' parameter was removed from the '_build_and_record_step_prompt' function signature but was not used in the function body. This change is appropriate as it simplifies the function signature without affecting functionality.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The removal of the 'organization' parameter from several functions in 'agent.py' and 'agent_functions.py' seems intentional, as the parameter was not being used in the function bodies. This change simplifies the function signatures and reduces unnecessary complexity.
4. skyvern/forge/agent_functions.py:99
  • Draft comment:
    The 'organization' parameter was removed from the '_convert_svg_to_string' function signature but was not used in the function body. This change is appropriate as it simplifies the function signature without affecting functionality.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The removal of the 'organization' parameter from several functions in 'agent.py' and 'agent_functions.py' seems intentional, as the parameter was not being used in the function bodies. This change simplifies the function signatures and reduces unnecessary complexity.
5. skyvern/forge/agent_functions.py:178
  • Draft comment:
    The 'organization' parameter was removed from the '_convert_css_shape_to_string' function signature but was not used in the function body. This change is appropriate as it simplifies the function signature without affecting functionality.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The removal of the 'organization' parameter from several functions in 'agent.py' and 'agent_functions.py' seems intentional, as the parameter was not being used in the function bodies. This change simplifies the function signatures and reduces unnecessary complexity.
6. skyvern/forge/agent_functions.py:328
  • Draft comment:
    The 'organization' parameter was removed from the 'cleanup_element_tree_factory' function signature but was not used in the function body. This change is appropriate as it simplifies the function signature without affecting functionality.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    The removal of the 'organization' parameter from several functions in 'agent.py' and 'agent_functions.py' seems intentional, as the parameter was not being used in the function bodies. This change simplifies the function signatures and reduces unnecessary complexity.

Workflow ID: wflow_pRXNLQtzhr4DWPeK


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@msalihaltun msalihaltun merged commit d520254 into main Nov 25, 2024
2 checks passed
@msalihaltun msalihaltun deleted the salih/add-navigation-block branch November 25, 2024 16:37
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