Bug with node deletion when controlling node expansion

[mihna123]

Describe the bug This bug happens when the user wants to have all branches expanded and chooses to have a dynamic node adding/deleting system. By adding two or more nested branches and then deleting them an error will occur : "Node with id=x doesn't exist in the tree"

As an example i have created a sandbox. Clicking the button "add node" will add a node as a child to a node that has been selected, clicking the button "delete" will delete node that has been selected (and all it's children).

To Reproduce Steps to reproduce the behavior:

1. Go to this sandbox
2. Click on 'node 1'
3. Click on 'add node' button
4. Repeat until you have 3 levels of nested nodes (see screenshot)
5. Start deleting nodes from the lowest level up ('node 3', 'node 2', 'node 1')
6. See error

Expected behavior It is expected that you get an error "Node with id=x doesn't exist in the tree"

Screenshot This is the look of the tree at step number 4:

Desktop (please complete the following information):

• OS: [Windows 10]
• Browser [Firefox]
• Version [114.0.2]

[dgreene1]

@mihna123 I’m not used to seeing setData called in a recursive method. I’m concerned that that is the reason why you’re seeing this.

So can you rewrite your sandbox to utilize pure functions that calculate the new tree data and then call setData once and only once?

But maybe I’m reading your code incorrectly. So while you consider my suggestion, @kpustakhod can you speak with our PM about getting time to investigate this in a future sprint? I don’t think sprint 14.1 or 14.2 have room, so let’s ask for 14.3. @mihna123 for clarity that means that we won’t be able to consider your PR till at least 4 weeks from now.

[dgreene1]

@callwait the reason I didn’t merge the PR is because I think this is caused by an error in the consumer code. See above.

[totakoko]

I'm facing the same issue when manually changing the tree nodes and expanding all nodes.

Reproduction :

• go to this sandbox
• fill in the filter input with something like "node 1", that will remove half of the nodes (note that the filter is effective starting at 3 characters and expands all matching nodes)
• an error occurs because a node is not found

Does the PR #132 fix this issue?

[dgreene1]

@Ke1sy, @yhy-1, @kpustakhod please mention this to Kevin and Jason so it can be worked.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue was closed automatically since it was marked as stale because it has not had recent activity. Thank you for your contributions.

[lb-pendula]

For what it's worth, I was able to work around this bug by creating a hash of all my data IDs and abusing the key prop to force the component to re-mount to avoid that nasty internal state causing the crash.

[ivan75238]

For what it's worth, I was able to work around this bug by creating a hash of all my data IDs and abusing the key prop to force the component to re-mount to avoid that nasty internal state causing the crash.

Hi! Could you show your example of solving this problem?

[lb-pendula]

For what it's worth, I was able to work around this bug by creating a hash of all my data IDs and abusing the key prop to force the component to re-mount to avoid that nasty internal state causing the crash.

Hi! Could you show your example of solving this problem?

Basically this:

const MyTree = ({data, ...props}: ITreeViewProps) => {
  const treeKey = React.useMemo(() => data.map((i) => i.id).join('-'), [data])
  return <TreeView key={treeKey} data={data} {...props} />
}

[ivan75238]

For what it's worth, I was able to work around this bug by creating a hash of all my data IDs and abusing the key prop to force the component to re-mount to avoid that nasty internal state causing the crash.

Hi! Could you show your example of solving this problem?

Basically this:

const MyTree = ({data, ...props}: ITreeViewProps) => {
  const treeKey = React.useMemo(() => data.map((i) => i.id).join('-'), [data])
  return <TreeView key={treeKey} data={data} {...props} />
}

Thanks a lot! It works to me!

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue was closed automatically since it was marked as stale because it has not had recent activity. Thank you for your contributions.

[sjakos]

Used the hacky workaround from @lb-pendula after spending waaaay too much time trying to figure out why a filtered tree was causing errors.

[dgreene1]

@sjakos @ivan75238 @mihna123 @lb-pendula I’m sorry that it’s taken so long to get this bug looked at. I’m requesting our management to consider our volunteerism budget to use on this bug. It’s hard to rationalize spending budget on this when it’s a bug that we aren’t experiencing at our company. But we’ll try. Reopening the issue now.

[mellis481]

@mihna123 Calling flattenTree() more than once is potentially problematic. Iterating over the whole data set multiple times is inefficient and error prone.

When I made that small change (to only call it once), the issue you reported went away. https://codesandbox.io/p/sandbox/thirsty-black-2mmgjp (In this fork, you'll have to manually expand nodes after you add them)

[lb-pendula]

@dgreene1 Hey, just wanted to say that work would be appreciated! This project fills a real niche that exists for apps that (for legacy) reasons are stuck on React 16, and can't use the TreeView in newer versions of MUI.

[mihna123]

@mihna123 Calling flattenTree() more than once is potentially problematic. Iterating over the whole data set multiple times is inefficient and error prone.

When I made that small change (to only call it once), the issue you reported went away. https://codesandbox.io/p/sandbox/thirsty-black-2mmgjp (In this fork, you'll have to manually expand nodes after you add them)

@mellis481 I did the same thing without calling flattenTree(), redid the whole sandbox without using recursion as per @dgreene1 suggestion, the error persists. Here is the sandbox. The workaround from @lb-pendula works here as well.

The issue happens if the user wants to have all nodes expanded by default, so your code is not that relevant i think.

[mihna123]

@mellis481 I cant access your codesandbox, did you set it to public?

[mellis481]

@mihna123 Try now.

[mihna123]

@mellis481 Sandbox works now, thank you. So if I expand all nodes with expandedIds={treeData.map((x) => x.id)} the error happens again.

[mellis481]

@mellis481 Sandbox works now, thank you. So if I expand all nodes with expandedIds={treeData.map((x) => x.id)} the error happens again.

I just updated my sandbox to use a state variable to maintain expanded IDs and it works fine. I also tested the code you shared along with some console.logs and it looks like the component is re-rendering a bunch of times such that when it goes to delete the node, it has already been removed. So I think the way you've implemented the component in the codesandbox is the cause of the problem.

[mihna123]

@mellis481 Works better with a state variable. I am getting error now when trying to add a new node after deleting all nested nodes. See below for an example: How would you implemented it if you wanted a similar system?

[mellis481]

@mihna123 I don't have time at the moment to provide a full working codesandbox. I will say that, after spending a little bit of time trying to change tree data, it's not super easy to do it having to manage data in the state of the component that instantiates a tree.

Please take a look at this example, though, as it it's a different approach to dynamically changing tree data that may be helpful. https://dgreene1.github.io/react-accessible-treeview/docs/examples-MultiSelectCheckboxAsyncControlled

[mihna123]

@mellis481 Very interesting! I posted this issue a while back when working on a project I no longer work on, so I no longer have a personal interest in this. I will try to see if there is a valid PR I can provide since I have some time on my hands, but if you guys don't see how fixing this would benefit the project I'm fine with this issue being closed.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue was closed automatically since it was marked as stale because it has not had recent activity. Thank you for your contributions.