Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug with node deletion when controlling node expansion #133

Closed
mihna123 opened this issue Jun 29, 2023 · 26 comments · May be fixed by #174
Closed

Bug with node deletion when controlling node expansion #133

mihna123 opened this issue Jun 29, 2023 · 26 comments · May be fixed by #174
Labels
wontfix This will not be worked on

Comments

@mihna123
Copy link

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:

image

Desktop (please complete the following information):

  • OS: [Windows 10]
  • Browser [Firefox]
  • Version [114.0.2]
@dgreene1
Copy link
Owner

dgreene1 commented Jul 4, 2023

@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
Copy link
Owner

@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
Copy link

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
Copy link
Owner

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

@stale
Copy link

stale bot commented Oct 30, 2023

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 stale bot added the wontfix This will not be worked on label Oct 30, 2023
Copy link

stale bot commented Nov 6, 2023

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

@stale stale bot closed this as completed Nov 6, 2023
blittle added a commit to blittle/react-accessible-treeview that referenced this issue Feb 1, 2024
blittle added a commit to blittle/react-accessible-treeview that referenced this issue Feb 1, 2024
@dgreene1 dgreene1 reopened this Feb 1, 2024
@stale stale bot removed the wontfix This will not be worked on label Feb 1, 2024
@lb-pendula
Copy link

lb-pendula commented Feb 23, 2024

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
Copy link

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
Copy link

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
Copy link

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!

Copy link

stale bot commented Jun 11, 2024

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 stale bot added the wontfix This will not be worked on label Jun 11, 2024
Copy link

stale bot commented Jun 19, 2024

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

@stale stale bot closed this as completed Jun 19, 2024
@sjakos
Copy link

sjakos commented Jun 21, 2024

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
Copy link
Owner

@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.

@dgreene1 dgreene1 reopened this Jun 21, 2024
@stale stale bot removed wontfix This will not be worked on labels Jun 21, 2024
@mellis481
Copy link
Collaborator

mellis481 commented Jun 21, 2024

@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
Copy link

lb-pendula commented Jun 22, 2024

@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
Copy link
Author

mihna123 commented Jun 26, 2024

@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
Copy link
Author

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

@mellis481
Copy link
Collaborator

@mihna123 Try now.

@mihna123
Copy link
Author

mihna123 commented Jun 26, 2024

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

@mellis481
Copy link
Collaborator

mellis481 commented Jun 26, 2024

@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
Copy link
Author

@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:
acc-tree-view-error
How would you implemented it if you wanted a similar system?

@mellis481
Copy link
Collaborator

mellis481 commented Jun 26, 2024

@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
Copy link
Author

@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.

Copy link

stale bot commented Aug 26, 2024

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 stale bot added the wontfix This will not be worked on label Aug 26, 2024
blittle added a commit to blittle/react-accessible-treeview that referenced this issue Aug 26, 2024
Copy link

stale bot commented Sep 3, 2024

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

@stale stale bot closed this as completed Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants