🐛 [BUG]: Race condition during adding/removing nodes when hooking into change handlers

[SkyAphid]

Is there an existing issue for this?

• I have searched the existing issues and this is a new bug.

Current Behavior

This API is really amazing for what it does and for the most part has made my life extremely easy for what I set out to do. But the behaviors of removing nodes/edges has legitimately driven me insane for the entirety of my time with VueFlow. I may somehow just be using it wrong, but it's been wildly inconsistent.

All of the code for my post can be found here and you can launch the deployed version from its git page. I was able to mostly finish my program:
https://github.com/SkyAphid/corkboard

As far as I could tell by searching, no one else has brought up the problem I've been having. You can find the code I'm going to display below in its App.vue class, since I was able to find a decent solution around it for now.

My problem comes from deleting nodes. At first I thought it was as simple as calling removeNodes(id). The edges seemed to delete with it. However when I went to finally use my JSON files, I found that the edges weren't being removed, but hidden in the JSON file for some reason. I came back to edit the code, to find that removeEdges(id) wasn't working either when I tried to add it below removeNodes(). The node would disappear, but then pop right back up in the export, depending on how you ordered things (though I've forgotten what combination caused this, since I was so frustrated at the time and trying various things to just get it to work and move on).

I did experiments with using the edge.value filter functions, and applyEdgeChange, and still no luck. So I went back to trying to brute-force the removeEdges function to work. I was able to somewhat nail down the problematic behavior that way.

If you delete an edge, the node doesn't delete. If you delete a node, the edge doesn't delete. You can't do it at the same time for some reason. If I delete a node, its edges will still in the JSON output. If I try and do them at the same time, one or the other will stay hidden in the JSON output. Which resulted in this code, which "works:"

onNodeDoubleClick((nodeEvent) => {

  let pointerEvent = nodeEvent.event;
  let node = nodeEvent.node;

  pointerEvent.preventDefault();

  let items = [
    {
      label: "Delete Node", onClick: () => {

        removeNodes(node.id);

        let edges = getEdges;

        for (const edge of edges.value){
          if (edge.source == node.id || edge.target == node.id){
            removeEdges(edge.id);
          }
        }
        
      },
    },
  ];

etc, etc...

Calling this function will only delete one of the two at a time despite being in the same call. So I'll right click and tell it to delete the node, but only the edge will disappear. Then the next time, the node. But it gets even worse: If I have multiple edges connected to a node, it will only delete one edge at a time, then the next edge, then the next edge, and then finally it deletes the node.

Thankfully, I was able to get past the issue with the edges by doing this:

      label: "Delete Node", onClick: () => {

        removeNodes(node.id);

        let edges = getEdges;
        let edgesToRemove;

        for (const edge of edges.value){
          if (edge.source == node.id || edge.target == node.id){
            edgesToRemove.push(edge);
          }
        }
        
        removeEdges(edgesToRemove);

      },

So now at the very least, I have to click two times and it'll delete the edges, THEN the node on the second click. BUT it still asks me if I want to delete each node one by one. I honestly have no idea how to turn the confirmations off or define what warrants one. The least I could do was just make sure it's at least styled like the rest of the program.

I'm by far no expert in Vue/Javascript, I learned it along with this project as I went and built this in a week's time. But I think I've worked with it enough at this point to say that this is likely a bug of some kind.

Expected Behavior

Ideally, I don't want to have to call removeEdges at all. The removeNode function should frankly just delete any attached edges itself or be a toggleable setting. But I doubt that is something that will be compromised on, so at the very least I want to be able to call removeNode(id) and removeEdges(edges) in one call please, and they both disappear and I just go on with my day instead of having to repeat my inputs to get the desired result. Being able to control when the warning is displayed could help a lot too, since you don't really want users being able to control whether or not a disconnected/useless edge should be deleted or not.

Also, in my opinion, you guys should commit to edges working like everything else and not disappearing when a node is deleted because that strongly implies it's been deleted to new users. The resulting JSON output should be a representation of exactly what you see on the screen, with no surprises.

And as always, I know that maintaining APIs like this takes time and effort that I'm not entitled to. So I give you guys my upmost thanks for taking the time to read this long post and hopefully help me out. You're appreciated and I thank you very much!

Steps To Reproduce

I'll be deploying a version to my github page with the "fix" I was able to come up with. Simply delete a node with an edge attached to it to see the behavior.

Relevant log output

No response

Anything else?

Anyway, thank you so much for the amazing software! It really got my project off the ground and I was able to completely replace a commercial software that had gotten a little too expensive for us in a short amount of time. Hopefully this gets padded out so I can call it a day on this thing finally. I'm ready to get onto other things.

To whomever reads this, have a wonderful week/day/evening!

[bcakmakoglu]

Thanks for the long post and detailed report.

Please provide a minimal reproduction of the issue since it makes it a lot easier to actually debug the problem since a long message doesn't really give me much of an idea what's up and I don't have the time to research a full github repo ^^

You can find a template right here.

[bcakmakoglu]

@SkyAphid Any repro yet? :)

[SkyAphid]

I'm writing up something now, will post in a little bit

EDIT: Codesandbox is probably the buggiest, jankiest site I've ever worked with... So I'm just gonna pull and drop the files here when I finish.

[SkyAphid]

Alright. Sorry it took so long. Codesandbox.io was a nightmare to try and actually code in. Everytime I'd CTRL-S it'd keep reverting my code back and the preview would constantly shut down, etc. It got to the point where it just completely stopped working, so I just started working locally and I managed to get it done in a few minutes.

https://codesandbox.io/p/devbox/dank-tdd-flxh6h

Bug explanation

I copied the result back to sandbox, since that was your preferred place. I managed to discover more about the bug, as it seems (for me) to have originated from using the Confirm Delete Node example on the VueFlow website. If you don't use the confirm dialogs at all, it actually has the desired result. So that's what I may go for in my project, is just removing the dialogs to save the trouble.

The exact line of code that starts triggering the behavior is apply-default="false" in . You'll see in the example above that when you try to delete the node/edges at the same time, it'll do the edges and then the node itself. Depending on your perspective, this may not be a bug at all.

How I'd change it

I'd consider tweaking the way the dialogs/apply-default works to where if I've called both removeNodes/removeEdges, they're all added to a queue in one go. So if the dialog appears and I agree to delete them, it does it all at once instead of just one at a time.

Otherwise if you don't want to change anything, I'd suggest at the very least adding a note to the tutorial page that this behavior can occur. I'm pretty much the perfect example here, I learned VueJs/Javascript/CSS with this project, and didn't realize the implications of the dialog appearing like this.

Again, thanks for the software! I'll just have to make some adjustments to my program to take what I learned into account.

[bcakmakoglu]

it seems (for me) to have originated from using the Confirm Delete Node example on the VueFlow website

Sounds like the example causes you a race condition, which isn't what I want to happen but it can happen.
To solve this use-case more elegantly I'll be adding a onBeforeDelete handler in the next minor version which can be used for this case instead of doing the whole changes workaround which isn't as elegant and intuitive anyway (the whole concept of changes is most likely going to be trashed in VueFlow 2.x anyway).

I'll keep this issue open to track the implementation of onBeforeDelete though, thanks again for the report.

[SkyAphid]

No worries! I'll keep an out and good luck. If onEdgesChange/onNodeChanges is removed, please consider adding a built in undo/redo system in too then, since that's how I'm tracking them in my program right now.

[logaretm]

Hey, we ran into this recently while building an undo/redo feature, it used to work in older releases.

Having the deleted node in theonNodeChanges entry could help, or alternatively the onBeforeDelete can help us find the node before it gets removed.

I think having onNodeChanges running before the changes are processed might be a good idea in general. LMK if I can help PR either approach or test a nightly release for you.

[bcakmakoglu]

I think having onNodeChanges running before the changes are processed might be a good idea in general.

That is already what is happening, where did you get the impression that it would be otherwise?
If you handle changes "manually" (by setting apply-default="false") you have to apply the changes that are passed throug the change events, so changes can never be processed before they were even passed to you.

[logaretm]

That is already what is happening, where did you get the impression that it would be otherwise?

Its an observation, upgrading > 1.30 or so changed the timing for us. Before we used to be able to find the deleted node in the onNodeChanges handler. Then we couldn't, I'm not 100% on each release changes so take my words with grain of salt.

If you handle changes "manually" (by setting apply-default="false")

I have tried it and this works well for us, thank you and I appreciate taking the time to reply and maintain this project.

[bcakmakoglu]

Its an observation, upgrading > 1.30 or so changed the timing for us. Before we used to be able to find the deleted node in the onNodeChanges handler. Then we couldn't, I'm not 100% on each release changes so take my words with grain of salt.

If you can pinpoint which version introduces this issue for you and/or provide a minimal repro of the issue, I'd appreciate that a lot. Then I could figure out what changes I made that might have changed the behavior (if it really did 😅) and fix it ^^

[logaretm]

sure, I will try to pinpoint the release over the weekend.

[Q16solver]

@logaretm Hey, you said you were building something of an undo/redo feature, I was just looking to also do that and was surprised to find that it wasn't exactly already a plugin as a part of the core library. Perhaps the vue-flow library should have the node/edge changes stored as history and be able to undo/redo them? Seems like changes is going to be removed in the next major release though from the comment

[bcakmakoglu]

perhaps the vue-flow library should have the node/edge changes stored as history and be able to undo/redo them?

This will most likely not happen - VueFlow is concerned with providing you tools to build this but not with solving specific use-cases for you.
Might be I'll add a basic example for it to the docs though, we'll see.

Seems like changes is going to be removed in the next minor release though from the commnet

If they are removed it's gonna be with the next major update, not minor, since it'd be a breaking change.

[Q16solver]

Makes sense, I was thinking too close to editors when seeing examples like https://tiptap.dev/docs/editor/extensions/functionality/undo-redo, but simply having some basic examples would also be great, thanks for the explanation

[bcakmakoglu]

@logaretm any updates on the repro? :)

[logaretm]

@bcakmakoglu Sorry for the delay, I was traveling and couldn't find time to do it.

I believe I have found the versions where this change occurred for this. Finding the node during the onNodeChange callback with findNode function was possible in <= 1.21.x releases but after >= 1.22 it isn't possible anymore.

Here is a sandbox with minimal repro.

To reproduce it:

1. Delete any node.
2. Observe the console, it should have the node found and logged via the findNode API.

I realize this was a long time ago so probably it isn't worth reverting, the applyChanges API works great for us now with the applyDefault set to false.

Let me know if I can help more.

[bcakmakoglu]

I’ll check it out once I’m back from Japan 🫶