-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feat: Scope cut-copy-paste #215
Conversation
Update:
I feel it ready for merging, but there are still some potential directions for further optimization if we have time: For performance:
For code:
Known issues:
But they are not big deals, 1) they rarely happen, I just came across a few times and I can't reproduce them now because of the strict conditions 2). I add many try/catch clauses to handle them to protect the whole update process from blocking. I list them here just in case you come across them and let you know the bugs are introduced in this PR. |
* @returns | ||
*/ | ||
function createTemporaryNode(pod, position) { | ||
function createTemporaryNode(pod, position, parent = "ROOT", level = 0): any { |
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.
One bug: after copying a scope, the nodes inside the scope can be moved out. The reason is that the extent: "parent"
field is not set for the new scope.
For reference, this extent: "parent"
field is set when a pod is moved into a scope.
codepod/ui/src/lib/store/canvasSlice.tsx
Lines 608 to 617 in 5b39efe
let newNode: Node = { | |
...node, | |
position, | |
parentNode: scope.id, | |
extent: "parent", | |
data: { | |
...node.data, | |
level: scope.data.level + 1, | |
}, | |
}; |
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.
fixed
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.
I see. This may be the same reason why a pod can be moved out after refreshing the page. Let me take a look and fix the bug tonight.
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.
fixed
Yeah, the performance for coping a scope wasn't good. The scope is jumping with the cursor instead of moving smoothly.
I feel this wasn't the issue. Re-rendering all nodes is always necessary for a single node change because there's no way to update one single node in ReactFlow. So the root problem is that ReactFlow models the nodes as a list, not a hashmap. We'd probably have to drop Reactflow in the long run. |
I'll leave a few days for me to debug the performance. Then let's get this merged. |
Update: I added one line to fix the bug that a pod can be moved out of scope after refreshing the web page. If you want to test it, please restart the socket docker first. But it may be better to check the |
I’ve fixed it as well.. in case you missed my commit. |
Not the same bug, I fixed the one for refreshing. |
I see. Sounds good. |
Merging. Let's fix the performance later. Thanks, Xinyi! |
Other miscellaneous:
To-do: