Clicking on any disabled tab-toolbar item throws an error.

[kittaakos]

Bug Description:

Steps to Reproduce:

1. Open a workspace
2. Open the Search view
3. Check that the Clear Search Results is disabled. (It should be disabled)

4. Click on it. It throws an error.

ncaught (in promise) TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'TheiaDockPanel'
    |     property '_layout' -> object with constructor 'DockLayout'
    --- property '_parent' closes the circle
    at JSON.stringify (<anonymous>)
    at CommandRegistry.<anonymous> (command.ts:298)
    at step (command.ts:15)
    at Object.next (command.ts:15)
    at command.ts:15
    at new Promise (<anonymous>)
    at ../../packages/core/lib/common/command.js.__awaiter (command.ts:15)
    at CommandRegistry.../../packages/core/lib/common/command.js.CommandRegistry.executeCommand (command.ts:290)
    at TabBarToolbar._this.executeCommand (tab-bar-toolbar.tsx:184)
    at HTMLUnknownElement.callCallback (react-dom.development.js:188)

Additional Information

• Operating System:
• Theia Version:

[kittaakos]

Ohh, clicking on any disabled toolbar item throws an error.

[vince-fugnitto]

I've noticed the error before, it occurs when clicking a disabled toolbar item.

[akosyakov]

We should not serialize random args here: https://github.com/eclipse-theia/theia/blob/master/packages/core/src/common/command.ts#L298 Just drop args.

[kittaakos]

Just drop args.

Why don't we rebind JSON.stringfy with a lib that can handle cycles?

[kittaakos]

It's not just about whether we drop the args or not, but why does the command handler run when it is disabled?

[akosyakov]

Why don't we rebind JSON.stringfy with a lib that can handle cycles?

I don't think it's meaningful to log widgets.

[kittaakos]

I don't think it's meaningful to log widgets.

Sure, but it has a type or any[] and I want to see the details of the failed command in the logs.
Also, if we just drop the args from the log, it is still an error, we should not run the command when it is disabled.

[akosyakov]

Sure, but it has a type or any[] and I want to see the details of the failed command in the logs.

I think command id is good enough. It is not first time when we see such exceptions i would prefer to remove it.

Also, if we just drop the args from the log, it is still an error, we should not run the command when it is disabled.

this, i agree, your PR looks good

[kittaakos]

I think command id is good enough.

OK.

this, i agree, your PR looks good

Alright. Let's do that.