fix(dataviz): Don't delete node for note on update - #393

[AndrewNatoli]

Will resolve #393

Looks like as part of 0.7.2 a deleteNote call was added to setNote. (View changes)

Doing so causes graphlib to delete the inbound links from other nodes so when this node is re-created it will not have its inbound links. (View relevant graphlib source)

I switched the doDelete back to getNote so it still satisfies finding if the note already existed for the sake of triggering the onUpdate / onAdd events-- it seems to be behaving as expected.

That said, I noticed the original approach was to delete the outbound links for the note so they can be re-created. That looked like this:

if (noteExists) {	
      (this.graph.outEdges(id) || []).forEach(edge => {	
        this.graph.removeEdge(edge);	
      });	
    }

In that previous code, the actual node for the note was not being deleted nor its inbound references.

I can update this PR to restore that code if it seems necessary. Once I have feedback on that I'll check on the tests.

• Awaiting initial feedback
• Tests passing in foam-core
• Tests passing in foam-vscode

[riccardoferretti]

hi @AndrewNatoli, thanks for looking into this and spotting the issue with the inbound links.
The reason why the tests are failing because if we don't remove the outbound links, they will linger even after the note has been updated.
From this it will probably make sense to, in doDelete, set the note to null or undefined instead of removing it - this way graphlib will not eagerly remove the inbound links.
Also, we need to add a test case for the bug this PR is meant to fix.

[AndrewNatoli]

The reason why the tests are failing because if we don't remove the outbound links, they will linger even after the note has been updated.

Aha, I see that now. I'll try and get back to this a bit later today, thanks for confirming how that process works!

[AndrewNatoli]

How would you feel about lifting this:

(this.graph.outEdges(noteId) || []).forEach(edge => {
        this.graph.removeEdge(edge);
      });

out of doDelete and into removeForwardLinks? Figure the name lines up with getForwardLinks.

In setNote, removeForwardLinks would be called if the note exists and we wouldn't be mixing different actions in doDelete.

[riccardoferretti]

happy to go with that 👍

[riccardoferretti]

this would cause the inbound/outbound links to be removed, no?
I reckon we would need to set it to null to keep the existing inbound connections
(can you add a test case for that?)

[AndrewNatoli]

It looks like doDelete is now only being called when a note is deleted which I think means this shouldn't be a problem anymore.

If there's a concern about a deletion rippling to connections between neighboring files, those seem fine.

Here if I delete "types of food" the original connections in "soup" remain

[riccardoferretti]

if pasta has a connection to types-of-food, I would expect to see that connection even afterwards, but instead of being associated to a real note, it would be associated to a placeholder note

[AndrewNatoli]

Ah I see.

So... On delete, if a note has inbound links, do not entirely remove the node just set it back to a placeholder.

This seems to be an existing problem with the current release.

Mind if I add that to the issue tracker and try to address that separately? It doesn't look like it'll overlap with this change to update events.

I'm also having some recurring issues running the extension in dev mode that are making it a little tricky, I'll ask about those in Discord later and see if it's anything I can update the docs for as well.

[riccardoferretti]

I understand that your specific issue was with the removal of the inbound links, which you have now solved.
I see this issue as fixing a problem with the delete function, which has not been solved yet.
Is there a reason why you'd rather not make the change here?

Also, it's important to add the test cases for:

• the bug that was fixed
• the current incorrect behavior (the fact that tests are not failing now means we are not capturing this case)

I am also happy to contribute to the branch, for me what matters is merging a complete fix to the deletion bug.

[riccardoferretti]

hi @AndrewNatoli, thanks again for taking the initiative to fix this bug.

Let me know if my message on Discord helped you with the dev issues. The problem is caused by a mixture of commonjs and non-commonjs modules, so the fixes in tsconfig.json should do it. Having done various tests I am pretty comfortable that's the issue, but it was also sporadic before on my machine, so having a second report would be helpful.
Let me know, and if it works for you we can add it to this PR.

Also, let me know if you are happy to implement the changes I have requested, or you'd rather me doing it - whether on this branch or not (I will not publish a new release until all those issues are fixed though, so functionally it's the same).

I don't want to step on your work, but I understand you might be busy and I am keen to release a new version with the fix - let me know how you would like to proceed. Thanks!

[AndrewNatoli]

Hi @riccardoferretti - I was able to add a test for the new code which catches the regression with the previous code.

I'll try and revise the doDelete method later today (early afternoon here right now) -- I might have some time to hit it. If you need to guarantee it getting done I'm certainly open to you taking it, but I don't want to throw that on you unnecessarily.

--

• Add code to set to null
• Add tests

[AndrewNatoli]

This should be ready for a review now :D

[riccardoferretti]

Looks great!