Skip to content

Edges bug fix#53

Merged
srijanpatel merged 3 commits intomainfrom
EdgesBugFix
Dec 18, 2024
Merged

Edges bug fix#53
srijanpatel merged 3 commits intomainfrom
EdgesBugFix

Conversation

@preet-bhadra
Copy link
Copy Markdown
Collaborator

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

edges not appearing bug is now fixed, view button implemented, and python node works but with some constraints.


Important

Fixes edge visibility bug, adds view button, and improves node handling in DynamicNode.tsx and BaseNode.tsx.

  • Bug Fix:
    • Fixes edges not appearing in DynamicNode.tsx by improving predecessor node handling.
  • Features:
    • Adds a view button in BaseNode.tsx to open a modal for node output.
    • Implements NodeOutputModal in DynamicNode.tsx for displaying node output.
  • Code Improvements:
    • Refactors predecessor node computation in DynamicNode.tsx using useMemo for efficiency.
    • Updates DynamicNode.tsx to use useUpdateNodeInternals for node updates.

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

Copy link
Copy Markdown
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 48c3ba7 in 1 minute and 28 seconds

More details
  • Looked at 369 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. frontend/src/components/nodes/DynamicNode.tsx:264
  • Draft comment:
    Consider refactoring the logic for computing predecessorNodes to avoid redundancy. The logic in useState initialization and useMemo for finalPredecessors is similar and can be combined.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of useMemo for finalPredecessors is a good optimization to avoid unnecessary computations. However, the logic for updating predecessorNodes is duplicated in both useState initialization and useMemo. This can be refactored to avoid redundancy.
2. frontend/src/components/nodes/DynamicNode.tsx:146
  • Draft comment:
    Include nodeRef in the dependency array of this useEffect to ensure it runs when nodeRef changes.
  • Reason this comment was not posted:
    Comment was on unchanged code.
3. frontend/src/components/nodes/DynamicNode.tsx:335
  • Draft comment:
    Include connection in the dependency array of this useEffect to ensure it runs when connection changes.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.

Workflow ID: wflow_IhnHX3Ku9thIgHab


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

@srijanpatel srijanpatel merged commit 5e12707 into main Dec 18, 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.

2 participants