refactor Canvas #189

lihebi · 2023-01-02T19:48:09Z

Refactor:

[refactor 1] split node types into nodes/[Code|Scope|Rich].tsx.
[refactor 2] separate logics from the Canvas component into hooks:
- useCopyPaste: the copy paste cut logic
- useNodeLocation: provide checkNodesEndLocation to check if the node is dropped into a scope, if so, set parent and update levels.
- useNodeOperations: provide addNode to create new nodes
- useInitNodes: the logic to load nodes on entry. Steps:
  1. connect Yjs provider
  2. If Y.doc is inconsistent with database, use database to overwrite Y.doc
[refactor 3] cleaner separation among the following three data stores: (1) Yjs, (2) Zustand store, (3) database.
- the Canvas only operates on Yjs'snodesMap (with a few exceptions e.g., addPod)
- The nodesMap operation (add, delete, update(x,y,width,height,parent))) will trigger observer to push to Zustand store and remote database. Here's the logic:
  1. Always push to Zustand store
  2. When transaction.local === true && !node.data?.clientId (i.e., the change is made by the current user, and it is not a temporary pasting node), write to database.
  - Additional note: now this update(x,y,width,height,parent) observer will push node position and parent to Zustand store and database, so
    - there's no need to useEffect on xPos,yPos in e.g. CodeNode.
    - there's no need to setPodPosition and setPodParent in checkNodesEndLocation

Other small refactors:

use node.width instead of node.style.width
use node.data.level instead of node.level
deletePod(apolloClient, {id, toDelete}) no longer accepts toDelete.

li-xin-yi · 2023-01-02T23:26:46Z

ui/src/lib/nodes.tsx

@@ -128,19 +148,22 @@ export function useNodesStateSynced(nodeList) {
        } else if (change.action === "delete") {
          const node = change.oldValue;
          console.log("todelete", node);
-          deletePod(null, { id: node.id, toDelete: [] });
+          deletePod(apolloClient, { id: node.id, toDelete: [] });


Imagine the situation: If a node is deleted by a collaborator, a deletion operation is synced to nodesMap. On my side, I pulled the update from nodesMap, it will schedule a duplicate deletion request to DB. That's why I set the client to null, only update store.pods in local when detecting any deletion from the remote side. The deletion in DB should only be requested by the user who explicitly deletes it.

Yes, I’m adding a condition and setting it to null for remote peers.

li-xin-yi · 2023-01-03T00:04:09Z

ui/src/lib/nodes.tsx

+            !node.data.hasOwnProperty("clientId") ||
+            node.data.clientId === clientId
+        )
+        .sort((a: Node, b: Node) => a.data.level - b.data.level)


Suggested change

.sort((a: Node, b: Node) => a.data.level - b.data.level)

.sort((a: Node, b: Node) => a.data.level - b.data.level)

.map((node) => ({

...node,

selected: selectedPods.has(node.id),

hidden: node.data?.hidden === clientId,

}))

and then you can re-use triggerUpdate in many places to replace setNodes, such as obverser of the nodesMap, selectPod, onFocus functions in RichNode and codeNode (which unselect all pods), etc.

Thanks, I've added this to triggerUpdate.

li-xin-yi · 2023-01-03T00:11:27Z

ui/src/components/Canvas.tsx

+      setPasting(null);
+      // delete the original (hidden) node
+      if (cutting) {
+        nodesMap.delete(cutting);


Suggested change

nodesMap.delete(cutting);

reactFlowInstance.deleteElements({ nodes: [{ id: cutting }] });

As I mentioned before, make sure the remote deletion only scheduled by the actual user who request to do that.

I feel deleteElements is the same thing as (and triggers) nodesMap.delete.

Not really. onNodesDelete only be called when deleteElements is explicitly called or nodes are deleting by shortcut (Del). From that, we can distinguish whether the deletion is issued by local user or remote peers.

I think onNodesChange covers onNodesDelete. I actually removed onNodesDelete.

I plan to use Yjs's transaction.origin (ref) to distinguish between local and remote edits.

I see, I think you can call remote deletions to DB only in onNodesChange here

codepod/ui/src/lib/nodes.tsx

Line 79 in abc81df

if (isNodeRemoveChange(change)) {

Then all deletions observed from nodesMap are synced from remote peers (yes maybe from myself, but deletePod will check if the pod exists first).

Anyway, it's up to you. But be careful, I don't know if the onNodesChange really exactly covers only all explicit deletions. I will test it when your PR finishes tomorrow.

I've finished this PR. Feel free to leave comments!

I see, I think you can call remote deletions to DB only in onNodesChange here

codepod/ui/src/lib/nodes.tsx

Line 79 in abc81df

if (isNodeRemoveChange(change)) {

Then all deletions observed from nodesMap are synced from remote peers (yes maybe from myself, but deletePod will check if the pod exists first).

I'm handling both zustand-store and database updates in Yjs observer (which is triggered when this user or other users make change to nodesMap). And I'm using onNodesChange only for updating nodesMap when there're reactflow operations.

lihebi · 2023-01-03T10:18:15Z

ui/src/components/Canvas.tsx

+      // NOTE we have to trigger an update here, otherwise the nodes are not
+      // rendered.
+      triggerUpdate();


This is actually where I use triggerUpdate. Without this, the canvas will not show anything.

lihebi · 2023-01-03T10:59:09Z

ui/src/components/nodes/Scope.tsx

+                // This does not work, will throw "Parent node
+                // jqgdsz2ns6k57vich0bf not found" when deleting a scope.
+                //
+                // nodesMap.delete(id);
+                //
+                // But this works:
+                reactFlowInstance.deleteElements({ nodes: [{ id }] });


@li-xin-yi nodesMap.delete and reactFlowInstance.deleteElements are indeed different, te latter seems to support recursive deletion of a scope.

lihebi added 2 commits January 2, 2023 11:46

refactor Canvas

8f1c6cc

check for null content before parse

abc81df

li-xin-yi reviewed Jan 3, 2023

View reviewed changes

lihebi added 2 commits January 3, 2023 01:58

update store & db inside reactflow provider

d6873b2

abstract triggerUpdate a little

0e424d6

lihebi commented Jan 3, 2023

View reviewed changes

lihebi changed the title ~~(WIP) refactor Canvas~~ refactor Canvas Jan 3, 2023

remove toDelete in deletePod

24e2f72

lihebi commented Jan 3, 2023

View reviewed changes

lihebi mentioned this pull request Jan 3, 2023

[WIP] cross-page scope cut-copy-paste #184

Closed

remove minHeight of ResultBlock

794c5bc

lihebi merged commit d61f9c7 into main Jan 3, 2023

lihebi deleted the refactor-canvas branch June 30, 2023 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor Canvas #189

refactor Canvas #189

lihebi commented Jan 2, 2023 •

edited

Loading

li-xin-yi Jan 2, 2023

lihebi Jan 3, 2023

li-xin-yi Jan 3, 2023

lihebi Jan 3, 2023

li-xin-yi Jan 3, 2023

lihebi Jan 3, 2023

li-xin-yi Jan 3, 2023 •

edited

Loading

lihebi Jan 3, 2023

li-xin-yi Jan 3, 2023 •

edited

Loading

lihebi Jan 3, 2023

lihebi Jan 3, 2023

lihebi Jan 3, 2023

lihebi Jan 3, 2023

-        .sort((a: Node, b: Node) => a.data.level - b.data.level)
+        .sort((a: Node, b: Node) => a.data.level - b.data.level)
+        .map((node) => ({
+            ...node,
+            selected: selectedPods.has(node.id),
+            hidden: node.data?.hidden === clientId,
+          }))

	nodesMap.delete(cutting);
	reactFlowInstance.deleteElements({ nodes: [{ id: cutting }] });

refactor Canvas #189

refactor Canvas #189

Conversation

lihebi commented Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

li-xin-yi Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

li-xin-yi Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihebi commented Jan 2, 2023 •

edited

Loading

li-xin-yi Jan 3, 2023 •

edited

Loading

li-xin-yi Jan 3, 2023 •

edited

Loading