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

[tree] Child node 'path/to/my/file.ext' does not belong to this 'WorkspaceNodeId' tree #6688

Closed
kittaakos opened this issue Dec 4, 2019 · 10 comments
Assignees
Labels
bug bugs found in the application navigator issues related to the navigator/explorer 🤔 needs more info issues that require more info from the author

Comments

@kittaakos
Copy link
Contributor

Description

I can see multiple Child node 'path/to/my/file.ext' does not belong to this 'WorkspaceNodeId' tree errors in the console after updating to next@0.14.0-next.d5c81105 (d5c8110).

This seems to be a regression after #6679. (CC: @akosyakov)

Related change:
5b45e12#diff-c4f0263c39593601b3f84451c2de4106R281-R283

Reproduction Steps

Start the application.

OS and Theia version:
d5c8110

Diagnostics:

@kittaakos
Copy link
Contributor Author

this.nodes[root.id] !== root is always false:
Screen Shot 2019-12-04 at 11 47 31

@akosyakov
Copy link
Member

@kittaakos Do you know how to reproduce it from sources? the issue is that we used to reset the tree with some old state, now we don't reset it, but report as an error that it has to be fixed by the developer.

I don't think we want to go back with resetting some old root.

@akosyakov akosyakov added 🤔 needs more info issues that require more info from the author bug bugs found in the application navigator issues related to the navigator/explorer labels Dec 4, 2019
@kittaakos
Copy link
Contributor Author

kittaakos commented Dec 4, 2019

Do you know how to reproduce

Not yet. If I put a breakpoint and stop for a second or two here:

return {
id: WorkspaceNode.id,
name: multiRootName || WorkspaceNode.name,
parent: undefined,
children: [],
visible: false,
selected: false
};

I do not see the error. Otherwise, it pops up twice per each app start.

Edit: I was wrong, I can see the same error.

@akosyakov
Copy link
Member

akosyakov commented Dec 4, 2019

I do not see the error. Otherwise, it pops up twice per each app start.

Do you start the browser example from theia repo and as a workspace using this repo? I'm looking for a way to reproduce it. You can debug how many times NavigatorMode.updateRoot is called and from there.

Popping up twice also does not sound bad, before it could go in the infinite loop. Maybe we should just remove console.error and wait till someone reports the real functional issue. You don't see any real issues for the navigator from the user perspective, correct? cc @svenefftinge

@kittaakos
Copy link
Contributor Author

You don't see any real issues

Correct, I don't.

I will take care of the investigation as it does not happen from the Theia source but from a downstream project. 😕

@kittaakos kittaakos self-assigned this Dec 4, 2019
@akosyakov
Copy link
Member

akosyakov commented Dec 4, 2019

Ok, please check why it happens that the navigator root replaced twice for you on the startup. It should not happen without explicit workspace changes from the user. Also check that your code does not keep references to navigator nodes and don't pass them to the navigator model after the navigator root is changed.

@kittaakos
Copy link
Contributor Author

twice for you on the startup

Once the model initialization, the second time is the layout restorer.

Also check that your code does not keep references to navigator nodes

I am not aware of any tree/tree-model customizations. It rather seems like a timing issue that I cannot reproduce in Theia. See the GIF, the roots are out of sync. I am still on this issue, as from time to time, the navigator is empty 😕
screencast 2019-12-04 15-52-13

@kittaakos
Copy link
Contributor Author

This is not a Theia bug but was in my code. The missing return messed up the app state, and 'initialized_layout' was resolved before the layout was restored.

Bad code:

@injectable()
export class MyCustomFrontendApplication extends FrontendApplication {

    protected async initializeLayout(): Promise<void> {
       /*no return here!!!*/ super.initializeLayout().then(() => {
            // Do some custom things here
        });
    }

}

Good code:

@injectable()
export class MyCustomFrontendApplication extends FrontendApplication {

    protected async initializeLayout(): Promise<void> {
        return super.initializeLayout().then(() => {
            // Do some custom things here
        });
    }

}

Sorry for the noise, with the fix, it works as expected.

@akosyakov
Copy link
Member

So then we keep this error message. It seems to be helpful :)

@kittaakos
Copy link
Contributor Author

It seems to be helpful :)

It is 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application navigator issues related to the navigator/explorer 🤔 needs more info issues that require more info from the author
Projects
None yet
Development

No branches or pull requests

2 participants