TreeView reveal creates a race condition with TreeDataProvider#getChildren

[EhabY]

If the tree provider is loading the tree and TreeView#reveal is revealing an element that is not loaded yet (so the parents are loaded) then a race condition can cause the view to break with the message "Element with id X is already registered".

I have also tried to create a promise queue as suggested in this comment but the issue still exists. I think this issue is caused by reveal and refresh of the tree, like it's not reveal statements causing it but rather the tree is not synchronized with reveal.

See the following example repo (based on the tree view sample): https://github.com/EhabY/TreeViewRevealExample/tree/main

1. Run the extension
2. Run the command Test View: Reveal

2023-09-04.17-54-20.mp4

For reference this what the executed command does:

// refresh the parent
provider.refresh();

// Reveal the child which will load the parent
this.promiseQueue = this.promiseQueue.then(async () => view.reveal({ key: "aa" }, { focus: true, select: false }));

// refresh the parent
provider.refresh();

[EhabY]

Pinging @alexr00 so it does not get lost 🙏

[alexr00]

@EhabY the way for you to solve this from your end would be to keep track of if your tree is currently refreshing and then wait to do the reveal until after the refreshing has finished.

[EhabY]

@alexr00 How is it possible to track if the tree is refreshing? IIRC the refresh events do not return a promise so I would need to keep track of all visible nodes and then make sure that all of those open nodes have been reloaded (making sure to handle deleted nodes). Is there a better approach for this?

But even then, this would reduce the possibility of it happening instead of eliminating it which requires internal syncing.

[alexr00]

How is it possible to track if the tree is refreshing?

I was thinking you could track if you have an unfinished getChildren method that was called.

[EhabY]

Ah, I actually tried that but it didn't work because refreshing would call getChildren sequentially:

1. getChildren(undefined)
2. getChildren(root)
3. getChildren(child1)
   ...

So while we can easily check if a getChildren is being executed, we cannot know for sure when it has finished executing all calls unless we manually track all visible tree items (which can be error prone..)

[alexr00]

So while we can easily check if a getChildren is being executed, we cannot know for sure when it has finished executing all calls unless we manually track all visible tree items (which can be error prone..)

I think this is still possible. You just need to increment a counter when getChildren is called, then decrement it before returning.

[EhabY]

I just attempted a solution as you described and it still fails, I have added a counter that is incremented first thing in getChildren and a decremented last thing in getChildren and I get the following trace.

"Before" is printed first thing in getChildren, "During" is in the middle,"After" is printed last thing, "Done!" is printed when the counter is 0 and the created promise when refreshing is resolved.

Before:  0
During:  1
After:  0
Done! -- Finishes after only refreshing the root and not the rest of the tree
Before:  0
Before:  1
During:  2
After:  1
Before:  1
During:  2
After:  1
During:  1
After:  0
Done!

Notice how when I execute the command above, it refreshed the top level first (and finishes execution) before refreshing the rest of the tree - thus depending on the counter won't work as it finishes a refresh of the root before refreshing the rest.