xyflow / xyflow

React Flow | Svelte Flow - Powerful open source libraries for building node-based UIs with React (https://reactflow.dev) or Svelte (https://svelteflow.dev). Ready out-of-the-box and infinitely customizable.
https://xyflow.com
MIT License
21.54k stars 1.43k forks source link

Restoring a graph with dynamic handles works sparodically #4216

Closed DenizUgur closed 2 weeks ago

DenizUgur commented 2 weeks ago

Describe the Bug

This one is a bit hard to explain becuase I can only reproduce this on my app and it might be releated to something else. The reason I'm opening this issue is to ask for assistance on how to investigate this bug deeper.

My graph has nodes that populate their handles dynamically. If I want to restore the graph, I know what those handles are with their ids. I've followed the "Save / Restore" example on reactflow's website and came up with a callback like the one below. I call this function on ReactFlow's onInit. All of the time graph is populated correctly but sometimes I get an error like Couldn't create edge for target handle id: "rbDf66tJ", edge id: z8wkbkM8. and initial fitView is not entirely accurate (as in all nodes are in view but it's not same as clicking the fit view button on the graph).

I suspected this was due to reactflow not knowing handles while setting the edges on the next render so I've supplied them though internalSymbols but still I get this error sometimes.

Oh and also using setTimeout(..., 200) for setEdges seems to be working but that feels a bit hacky and edges gets rendered delayed.

This also happens only during development (without StrictMode)

What can I do to investigate what is causing this? Maybe there's an edge case on how reactflow checks the validity of the edges.

const resetGraph = useCallback((nodes, edges) => {
    const restore = async () => {
      setNodes(
        nodes.map((node) => {
          return {
            id: node.id,
            type: node.type,
            dragHandle: ".node-drag-handle",
            position: node.position,
            data: { node },
            [internalsSymbol]: {
              handleBounds: {
                source: Object.keys(node.output).map((pid) => ({
                  id: pid,
                  position: Position.Right,
                  x: 1,
                  y: 1,
                  width: 1,
                  height: 1,
                })),
                target: Object.keys(node.input).map((pid) => ({
                  id: pid,
                  position: Position.Left,
                  x: 1,
                  y: 1,
                  width: 1,
                  height: 1,
                })),
              },
            },
          };
        })
      );
      setEdges(
        edges.map(({ id, from, to }) => ({
          id,
          source: from.id,
          sourceHandle: from.pid,
          target: to.id,
          targetHandle: to.pid,
        }))
      );
      updateNodeInternals(nodes.map((n) => n.id));
    };

    restore();
  },
  [setEdges, setNodes, updateNodeInternals]
);

Your Example Website or App

No response

Steps to Reproduce the Bug or Issue

N/A at this moment

Expected behavior

I expect that there should be a way to initialize the graph with nodes, their handles, and edges at the same time. More precisely, "Don't do any checks or anything because I'm supplying everything you need"

Screenshots or Videos

No response

Platform

Additional context

No response

peterkogo commented 2 weeks ago

One of the things React Flow solves is that you can position your Handles with CSS and RF will do the rest.

You can simply create a custom node with a variable amount of <Handle /> components inside. Position them with CSS. Lean back.

Writing data into the internalsSymbol will almost always break something.

DenizUgur commented 2 weeks ago

Yup, the internalsSybmol stuff was my attempt to remove the warnings. But regardless, the warnings continue. I'm creating the handles within my nodes during render but I don't understand why I still get these warnings. Since I don't get any warnings if I use setTimeout(setEdges(...), 200), I thought that maybe reactflow doesn't know the handles during the next render.

Any additional debug information would go a long way but afaik reactflow doesn't output any debug logs. My theory with this bug/mistake is that handles aren't ready in the next render so the edges fail to connect sometimes. That's why I thought if I can tell reactflow that these handles exists and updateNodeInternals when they are rendered.

Is there a better way to achieve this? More precisely to inform reactflow about the entire flowchart with handles and all?

moklick commented 2 weeks ago

Is this just about warnings or are the edges not rendered correctly? If the rendered output is correct, you don't need to worry about the warning.

In React Flow 12 you will be able to pass a handles array to a node and supply all information needed to render a flow without measurements from library side (this was necessary to support SSR). You can find all details here: https://github.com/xyflow/xyflow/discussions/3764

DenizUgur commented 2 weeks ago

Well sometimes they are not rendered but if V12 supports passing on handles then maybe it won't be an issue. I'll upgrade and report back.

DenizUgur commented 2 weeks ago

Still continues. Unless I add a delay, it won't clear the warning and this issue affects rendering sparodically.

@moklick How can we debug this further?

the handles and edges I pass to setNodes and setEdges

image

what I see on nodes and edges (the ones the graph uses) right before rendering ReactFlow

image

[!NOTE] Both images taken at different instances so ids are different

[!IMPORTANT] Also note that warning states source for BB68hKjz

updated setNodes

import type { Node as XYFlowNode } from "@xyflow/react"
...
setNodes(
  nodes.map((node) => {
    if (!node.position) throw new Error("Node does not have a position")
    const { x, y } = node.position

    const sourceHandles: XYFlowNode["handles"] = Object.keys(node.output).map((pid) => ({
      id: pid,
      type: "source",
      position: Position.Right,
      x,
      y
    }))
    const targetHandles: XYFlowNode["handles"] = Object.keys(node.input).map((pid) => ({
      id: pid,
      type: "target",
      position: Position.Left,
      x,
      y
    }))

    return {
      id: node.id,
      type: node.type,
      dragHandle: ".node-drag-handle",
      position: node.position,
      data: { node },
      handles: sourceHandles.concat(targetHandles)
    }
  })
  )
moklick commented 2 weeks ago

If you want to render a node immediately, you need to set width and height attributes.

However I am not sure if that's really the issue here. Could you create a codesandbox for this issue https://new.reactflow.dev ?

DenizUgur commented 2 weeks ago

While trying to create the sandbox, I've found the problem. Even thought reactflow knew the handles, I didn't rendered them properly. The updated handles were coming from a different source which made it unavailable during next render. No problems with reactflow, sorry for taking your time :)

moklick commented 2 weeks ago

I am glad you could solve it 🙏