-
Notifications
You must be signed in to change notification settings - Fork 83
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
Conversation
…n and input handling improvements
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.
❌ Changes requested. Reviewed everything up to 8018578 in 1 minute and 52 seconds
More details
- Looked at
1308
lines of code in15
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:
Ensureinput_schema
keys match the expected input structure. The keyinput_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:
Ensuresource_handle
andtarget_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:
Replaceconsole.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:
Replaceconsole.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:
Replaceconsole.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.
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 a302941 in 29 seconds
More details
- Looked at
26
lines of code in1
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 ofeval
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.
backend/example_router.json
Outdated
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.
@preet-bhadra looks like you included testing file in the commit.
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 cf76bad in 33 seconds
More details
- Looked at
141
lines of code in1
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 namesetPredcessorNodes
. Consider renaming tosetPredecessorNodes
. 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:
UsingJSON.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.
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 faf226f in 33 seconds
More details
- Looked at
121
lines of code in1
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.
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 e0ec32a in 35 seconds
More details
- Looked at
55
lines of code in2
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.
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 28f5261 in 57 seconds
More details
- Looked at
95
lines of code in1
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.
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 184ad9f in 24 seconds
More details
- Looked at
51
lines of code in1
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 theconnect
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.
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.
❌ Changes requested. Incremental review on f4e3237 in 41 seconds
More details
- Looked at
13
lines of code in1
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.
…ge connection handling
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 b9e2d96 in 1 minute and 2 seconds
More details
- Looked at
821
lines of code in6
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 addingdispatch
to the dependency array to ensure the latest reference is used. - Reason this comment was not posted:
Confidence changes required:50%
TheuseEffect
dependency array on line 88 should includedispatch
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 fromroutes
toroute_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:
EnsurerouteMap
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 ofRoute_
withroute
for bothsourceHandle
andtargetHandle
to avoid mismatches. - Reason this comment was not posted:
Comment did not seem useful.
5. frontend/src/store/flowSlice.ts:154
- Draft comment:
Check ifinput_schema
already contains the keys fromoutput_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:
- 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.
… on condition row widths
…ptions in the select dropdown
…ourceNode's output schema keys
…path for accessing nested dictionary values
…when the output_schema property changes for predecessors
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 9778e44 in 30 seconds
More details
- Looked at
75
lines of code in1
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 ofJSON.parse(JSON.stringify(...))
, such aslodash.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:
Ensurelodash
is included in the project dependencies sincelodash.isEqual
is used for deep comparison of schemas. - Reason this comment was not posted:
Confidence changes required:50%
The code useslodash.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.
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 0208202 in 27 seconds
More details
- Looked at
32
lines of code in1
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 theoutput
prop passed toNodeOutputDisplay
is consistent with the expected data structure. Consider usingdata.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.
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 efa241f in 1 minute and 3 seconds
More details
- Looked at
418
lines of code in3
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 wheremodel_id
is not found inmodel_mapping
by raising an exception or returning a default value to avoid returningNone
. - 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%
Theprint_error
function intest_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:
EnsureselectedKeys
is used correctly. It expects an array, butcurrentValue
is a single string. Consider wrappingcurrentValue
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.
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.
❌ Changes requested. Incremental review on 4951d70 in 43 seconds
More details
- Looked at
21
lines of code in1
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.
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 2b8c1a8 in 45 seconds
More details
- Looked at
29
lines of code in1
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 addingMergeNode
toDEPRECATED_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.
Fixed Router node (earlier ifelsenode) post MISO implementation.
Important
Refactor
IfElseNode
toRouterNode
with enhanced routing logic and UI support across backend and frontend components.IfElseNode
toRouterNode
inrouter.py
, updating class names and logic for routing based on conditions.node_types.py
to replaceIfElseNode
withRouterNode
in supported node types.source_handle
andtarget_handle
toWorkflowLinkSchema
inworkflow_schemas.py
.RouterNode
inexample_router.json
.IfElseNode
withRouterNode
in components likeFlowCanvas.tsx
,RunViewFlowCanvas.tsx
, andDynamicNode.tsx
.RouterNode.tsx
to handle route conditions and UI interactions.NodeSidebar.tsx
andRouterEditor.tsx
to support route editing and display.useSaveWorkflow.ts
to handle new node and edge configurations.flowSlice.ts
to manage state changes related toRouterNode
and handle renaming of node titles and edges.This description was created by for 2b8c1a8. It will automatically update as commits are pushed.