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

Logic node post miso #58

Merged
merged 56 commits into from
Dec 25, 2024
Merged

Logic node post miso #58

merged 56 commits into from
Dec 25, 2024

Conversation

preet-bhadra
Copy link
Collaborator

@preet-bhadra preet-bhadra commented Dec 20, 2024

Fixed Router node (earlier ifelsenode) post MISO implementation.


Important

Refactor IfElseNode to RouterNode with enhanced routing logic and UI support across backend and frontend components.

  • Backend:
    • Rename IfElseNode to RouterNode in router.py, updating class names and logic for routing based on conditions.
    • Update node_types.py to replace IfElseNode with RouterNode in supported node types.
    • Add source_handle and target_handle to WorkflowLinkSchema in workflow_schemas.py.
    • Add example configuration for RouterNode in example_router.json.
  • Frontend:
    • Replace IfElseNode with RouterNode in components like FlowCanvas.tsx, RunViewFlowCanvas.tsx, and DynamicNode.tsx.
    • Update RouterNode.tsx to handle route conditions and UI interactions.
    • Modify NodeSidebar.tsx and RouterEditor.tsx to support route editing and display.
    • Adjust useSaveWorkflow.ts to handle new node and edge configurations.
    • Update flowSlice.ts to manage state changes related to RouterNode and handle renaming of node titles and edges.

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

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 8018578 in 1 minute and 52 seconds

More details
  • Looked at 1308 lines of code in 15 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. backend/app/nodes/logic/router.py:39
  • Draft comment:
    Ensure input_schema keys match the expected input structure. The key input_node might not align with the example JSON.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. backend/app/schemas/workflow_schemas.py:58
  • Draft comment:
    Ensure source_handle and target_handle are consistently set and used across the codebase to avoid connection issues.
  • Reason this comment was not posted:
    Comment did not seem useful.
3. frontend/src/components/Header.tsx:253
  • Draft comment:
    Replace console.log with proper logging to avoid performance issues in production.
  • Reason this comment was not posted:
    Marked as duplicate.
4. frontend/src/components/canvas/FlowCanvas.tsx:195
  • Draft comment:
    Replace console.log with proper logging to avoid performance issues in production.
  • Reason this comment was not posted:
    Marked as duplicate.
5. frontend/src/hooks/useSaveWorkflow.ts:88
  • Draft comment:
    Replace console.log with proper logging to avoid performance issues in production.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_c5IdPpvYLLySVP2a


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.

backend/app/nodes/dynamic_schema.py Outdated Show resolved Hide resolved
backend/app/nodes/dynamic_schema.py Outdated Show resolved Hide resolved
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 a302941 in 29 seconds

More details
  • Looked at 26 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. backend/app/nodes/dynamic_schema.py:36
  • Draft comment:
    Consider enhancing the error message to include the list of supported types for better clarity.
raise ValueError(f"Unsupported type '{value}' in schema for field '{key}'. Supported types are: {list(type_mapping.keys())}")
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of eval was removed, which is a good security improvement. However, the error message could be more informative by listing the supported types.

Workflow ID: wflow_VrdaMbMiVfFLKys1


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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@preet-bhadra looks like you included testing file in the commit.

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 cf76bad in 33 seconds

More details
  • Looked at 141 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/components/nodes/logic/RouterNode.tsx:71
  • Draft comment:
    Typo in function name setPredcessorNodes. Consider renaming to setPredecessorNodes. This typo is present in multiple places.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
2. frontend/src/components/nodes/logic/RouterNode.tsx:111
  • Draft comment:
    Using JSON.stringify for deep comparison can be inefficient. Consider using a deep comparison function for better performance.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_ZroKd3U5GNSXtPsE


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 faf226f in 33 seconds

More details
  • Looked at 121 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_9j5IT1IEJgCie691


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 e0ec32a in 35 seconds

More details
  • Looked at 55 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. backend/app/execution/workflow_executor.py:70
  • Draft comment:
    Replace print statements with proper logging for better production-level code management.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The print statements are used for debugging purposes and should be replaced with proper logging for production code.
2. backend/app/execution/workflow_executor.py:119
  • Draft comment:
    Replace print statements with proper logging for better production-level code management.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The print statements are used for debugging purposes and should be replaced with proper logging for production code.
3. backend/app/execution/workflow_executor.py:123
  • Draft comment:
    Replace print statements with proper logging for better production-level code management.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The print statements are used for debugging purposes and should be replaced with proper logging for production code.
4. backend/app/nodes/logic/router.py:136
  • Draft comment:
    Replace print statements with proper logging for better production-level code management.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The print statements are used for debugging purposes and should be replaced with proper logging for production code.
5. backend/app/nodes/logic/router.py:150
  • Draft comment:
    Replace print statements with proper logging for better production-level code management.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The print statements are used for debugging purposes and should be replaced with proper logging for production code.

Workflow ID: wflow_OsfqMyImEhLlXCri


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 28f5261 in 57 seconds

More details
  • Looked at 95 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/hooks/useSaveWorkflow.ts:71
  • Draft comment:
    Ensure 'routes' is defined in 'NodeData' or handle the case where 'routes' might be undefined to prevent runtime errors.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. frontend/src/hooks/useSaveWorkflow.ts:86
  • Draft comment:
    Consider validating 'title' and 'type' to ensure they are non-empty strings before using them for 'new_id'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The code already has a safe fallback mechanism. Empty strings will be handled by the || operator, falling back to 'Untitled'. The validation suggested wouldn't add meaningful safety. The current implementation is idiomatic JavaScript and follows common patterns for handling optional strings.
    Perhaps there could be edge cases where non-string values are passed that could cause runtime errors. The types suggest this shouldn't happen, but TypeScript types aren't always enforced at runtime.
    The TypeScript types show that title is optional but string, and type is required and string. The code is in a TypeScript file with proper typing, so these concerns are already handled by the type system.
    Delete the comment. The code already handles empty values appropriately through the || operator, and type safety is ensured through TypeScript.
3. frontend/src/hooks/useSaveWorkflow.ts:110
  • Draft comment:
    Add checks to ensure 'sourceNode' and 'targetNode' are not undefined before accessing their properties.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_ll7LoZoAfMO33NCo


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 184ad9f in 24 seconds

More details
  • Looked at 51 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/store/flowSlice.ts:132
  • Draft comment:
    Consider using a Set to track unique edges for better performance and readability when removing duplicates.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The logic for removing duplicate edges is correct, but it could be optimized for readability and performance by using a Set to track unique edges.
2. frontend/src/store/flowSlice.ts:157
  • Draft comment:
    Consider using a Set to track unique connections for better performance and readability when checking for duplicates.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The check for duplicate connections in the connect reducer is correct, but the logic could be optimized for readability and performance by using a Set to track unique connections.

Workflow ID: wflow_hqdb568JGdTTH1lb


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. Incremental review on f4e3237 in 41 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_0P6ck1NBfEThLfzR


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.

frontend/src/store/flowSlice.ts Show resolved Hide resolved
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 b9e2d96 in 1 minute and 2 seconds

More details
  • Looked at 821 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/components/nodes/logic/RouterNode.tsx:88
  • Draft comment:
    Consider adding dispatch to the dependency array to ensure the latest reference is used.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The useEffect dependency array on line 88 should include dispatch to ensure that the function is not stale and always has the latest reference.
2. frontend/src/components/nodes/nodeSidebar/NodeSidebar.tsx:363
  • Draft comment:
    Update the key from routes to route_map to maintain consistency.
              handleInputChange('route_map', newRoutes);
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/components/nodes/nodeSidebar/RouterEditor.tsx:62
  • Draft comment:
    Ensure routeMap is treated as an object throughout the component to avoid runtime errors.
  • Reason this comment was not posted:
    Comment did not seem useful.
4. frontend/src/hooks/useSaveWorkflow.ts:105
  • Draft comment:
    Ensure consistent replacement of Route_ with route for both sourceHandle and targetHandle to avoid mismatches.
  • Reason this comment was not posted:
    Comment did not seem useful.
5. frontend/src/store/flowSlice.ts:154
  • Draft comment:
    Check if input_schema already contains the keys from output_schema before updating to avoid redundant updates.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  1. The code uses object spread to merge schemas which already handles duplicates by overwriting existing keys. 2. Checking for existing keys first would add complexity without clear benefit. 3. Even if keys exist, we may want to update them with new values. 4. The current implementation is simple and correct.
    The comment could be pointing out a performance optimization. If schemas are large, avoiding unnecessary updates could be beneficial.
    The performance impact would be negligible in practice since schemas are typically small objects. The added complexity of checking keys first outweighs any theoretical performance benefit.
    The comment should be deleted. The current implementation using object spread is simple and correct, and adding key existence checks would introduce unnecessary complexity.

Workflow ID: wflow_Jl14DZnsfkggPOD2


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

…when the output_schema property changes for predecessors
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 9778e44 in 30 seconds

More details
  • Looked at 75 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/src/store/flowSlice.ts:213
  • Draft comment:
    Consider using a more efficient deep clone method instead of JSON.parse(JSON.stringify(...)), such as lodash.cloneDeep, to avoid performance issues and potential problems with non-serializable data.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
2. frontend/src/store/flowSlice.ts:249
  • Draft comment:
    Ensure lodash is included in the project dependencies since lodash.isEqual is used for deep comparison of schemas.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The code uses lodash.isEqual to compare schemas, which is a good practice for deep comparison. However, ensure lodash is included in the project dependencies.

Workflow ID: wflow_rQLDOiiD6fopvrBF


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 0208202 in 27 seconds

More details
  • Looked at 32 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/components/nodes/logic/RouterNode.tsx:428
  • Draft comment:
    Ensure that the output prop passed to NodeOutputDisplay is consistent with the expected data structure. Consider using data.run as it seems to be the intended output.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_38rKoTkaXHmgc9V6


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 efa241f in 1 minute and 3 seconds

More details
  • Looked at 418 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. backend/app/nodes/llm/llm_utils.py:179
  • Draft comment:
    Consider handling the case where model_id is not found in model_mapping by raising an exception or returning a default value to avoid returning None.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The current implementation using dict.get() returning None is a perfectly valid Python pattern. The method signature implicitly allows None returns. There's no strong evidence that this is causing issues. The calling code can handle None returns appropriately. Adding exception handling would make the code more complex without clear benefit.
    Maybe there are cases where getting an invalid model ID is truly exceptional and should be handled explicitly. The code could be more defensive.
    Returning None is a clear and idiomatic way to indicate "not found" in Python. Adding exceptions would complicate the code and force callers to handle them without providing significant benefit.
    The comment should be deleted. The current implementation using dict.get() returning None is a valid pattern that doesn't need modification.
2. backend/test_ollama.sh:26
  • Draft comment:
    The fancy error box might not display correctly in all terminal environments. Consider providing a simpler alternative or a flag to disable it.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The print_error function in test_ollama.sh uses a fancy box for error messages, which is visually appealing but might not be compatible with all terminal environments. Consider providing a simpler alternative or a flag to disable the fancy output.
3. frontend/src/components/nodes/nodeSidebar/NodeSidebar.tsx:319
  • Draft comment:
    Ensure selectedKeys is used correctly. It expects an array, but currentValue is a single string. Consider wrapping currentValue in an array if needed.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_e49YDcuFG1a1rsfA


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. Incremental review on 4951d70 in 43 seconds

More details
  • Looked at 21 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_pLwhGb3Kw589snjo


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.

frontend/src/components/nodes/InputNode.tsx Show resolved Hide resolved
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 2b8c1a8 in 45 seconds

More details
  • Looked at 29 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. backend/app/nodes/node_types.py:13
  • Draft comment:
    Consider adding MergeNode to DEPRECATED_NODE_TYPES if it's deprecated, or remove the comment if it's intended to be supported.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment is directly related to the change (commenting out MergeNode). It suggests a concrete action (either move to deprecated or remove comment). However, we don't know the author's intention - they may be temporarily commenting it out for testing, or may plan to remove it entirely later. The comment is asking for clarification rather than pointing out a definite issue.
    The comment is asking the author to clarify their intention, which violates our rules. We don't have enough context to know if MergeNode should be deprecated.
    While the suggestion could be helpful, the comment is essentially asking "what did you intend here?" which is exactly the kind of comment we want to avoid.
    Delete the comment since it's asking for clarification of intent rather than pointing out a definite issue.

Workflow ID: wflow_NpRsxxPDvkHZpahL


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

@srijanpatel srijanpatel merged commit 619b52b into main Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants