1675762 - Use AppErrorBoundary component in the Console panel

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Bug 1660435 introduced a new top level Error boundary component used to catch any exception that can happen during rendering.

When an exception happens the component renders the associated error message and stack trace together with a button allowing the user to file a bug.

The component is implemented in this file
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/AppErrorBoundary.js

We could generalize the component code and use it the Console panel too.

Some notes:

• We probably want to move the component file into devtools/client/shared/components directory (together with CSS). Where we should have the locale strings?
• The messaging should not explicitly mention "Net Monitor" when it renders an error within the Console panel
• Bugzilla link should point to the right bugzilla-component (Netmonitor or Console)

Honza

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Note that the console already has an error boundary for the Message component (https://searchfox.org/mozilla-central/rev/96e2c6e14998f38e419850d55d8a3d32a3fc244a/devtools/client/webconsole/components/Output/Message.js#209-260)

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Nicolas, does it make sense to have another - top level one? (and share the code base)

Honza

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

yes, I don't think it would be a problem, and that would cover all the other components but the Message ones.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@Cody, you have done great job with introducing the Error boundary component for the Network panel. Would you be interested in this one too?

Honza

Comment 5

[Cody Welsh]

@Honza sure thing - I wouldn't mind working on this one as well. :)

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Great, assigned to you. Thanks for helping with this!

Nicolas, can you please add a few pointers to the Console panel code base, so Cody knows where to start?
Thanks!

Honza

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #6)

Nicolas, can you please add a few pointers to the Console panel code base, so Cody knows where to start?

Sure!
So here's the top-level component https://searchfox.org/mozilla-central/source/devtools/client/webconsole/components/App.js
We may want to have error boundary for the SideBar (https://searchfox.org/mozilla-central/source/devtools/client/webconsole/components/SideBar.js), and maybe the input one (https://searchfox.org/mozilla-central/source/devtools/client/webconsole/components/Input/JSTerm.js)

Cody, don't hesitate to ask any question :)

Comment 8

[Cody Welsh]

Awesome, I'll look into all of these now and probably provide said questions thereafter. :)

Comment 9

[Cody Welsh]

Okay, so as an overview of the tasks to be done here:

• Pull the AppErrorBoundary code I wrote in the other bug out to the shared directory.
• Replace other error boundaries that might exist already(?)
• Add in the error boundaries for all desired levels and components.
• Relocate the CSS, and handle the refactoring of each component as necessary.

If that sounds like a plan, I'll go ahead and start. :) Otherwise, let me know what part of that process should be changed. Since the main logic is already tested and built, I don't think this will take quite as long, except perhaps for the refactoring (if something comes up there).

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Replace other error boundaries that might exist already(?)

Not sure about that one. The boundary we have for messages is pretty nice (I think), and already proven to be useful in a few breakages (i.e. people were filing actionable bugs). But if you find a way to keep the same look and behavior while using your new component, why not :)

Comment 11

[Cody Welsh]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)

Replace other error boundaries that might exist already(?)

Not sure about that one. The boundary we have for messages is pretty nice (I think), and already proven to be useful in a few breakages (i.e. people were filing actionable bugs). But if you find a way to keep the same look and behavior while using your new component, why not :)

Hey, less work! Might as well keep it unless we need to do something otherwise if that's the case. :)

Everything else look good here?

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Everything else look good here?

Yes!

Comment 13

[Cody Welsh]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)

Everything else look good here?

Yes!

Alright, will start implementing this then. Questions about details inevitably to follow :)

Comment 14

[Cody Welsh]

Current issue I'm having trouble with: I can't seem to get the Sidebar to error out, but it does look like it's functioning properly with the Error Boundary surrounding it. My strategy thus far has been to modify the reducers in webconsole/reducers/ui.js to give null responses (e.g., on closing the sidebar), but I'm not sure what else I could do here. Any help for this would be appreciated! This is the strategy we used in the Network tab.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Cody Welsh from comment #14)

Current issue I'm having trouble with: I can't seem to get the Sidebar to error out, but it does look like it's functioning properly with the Error Boundary surrounding it. My strategy thus far has been to modify the reducers in webconsole/reducers/ui.js to give null responses (e.g., on closing the sidebar), but I'm not sure what else I could do here. Any help for this would be appreciated! This is the strategy we used in the Network tab.

What I did in the past was to throw in the render function of the component (e.g. in https://searchfox.org/mozilla-central/source/devtools/client/webconsole/components/SideBar.js#73 , add throw "ErrorBoundary check")

Hope this helps!

Comment 16

[Cody Welsh]

Oh, I'm kicking myself for not thinking of that! It would definitely make things easier to just manually throw an error in a child component. Thanks! This should speed things along.

Comment 17

[Cody Welsh]

Also, as Honza mentioned, where do you think we should now put the locale strings that are used in AppErrorBoundary?

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Cody Welsh from comment #17)

Also, as Honza mentioned, where do you think we should now put the locale strings that are used in AppErrorBoundary?

Other shared components are using devtools/client/locales/en-US/components.properties (e.g. https://searchfox.org/mozilla-central/rev/277ab3925eae21b419167a34624ec0ab518d0c94/devtools/client/shared/components/Frame.js#23-25), we can probably do the same here

Comment 19

[Cody Welsh]

Awesome, just wanted to make sure!

Secondary question: How do we want to handle all of the different CSS declarations for the different areas this component will be used in? We could just have the base AppErrorBoundary.css with specific declarations for everything, for example:

.network-monitor .app-error-panel {...}
...
.sidebar-error-boundary .app-error-panel {...}
...
.webconsole .app-error-panel {...}

I guess we could use the Network tab's error boundary CSS as a base, and declare specific styles as necessary for these others to also work. There are a bunch of other options here, though.

Comment 20

[Cody Welsh]

(I ended up abstracting a base set of styles out from those common between the Network panel boundary and the Sidebar boundary, which seems to be fine. Let me know if any other strategies are preferred)

Comment 21

[Cody Welsh]

One more thing that came up while moving over the localization stuff: Since we want the header to be correct for each component (e.g., "The [x] component has crashed"), is there a specific way we handle such cases? All I can think of is adding two localization strings (namely, "The" and "has crashed") so we can plug a componentName prop into the middle.

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Cody Welsh from comment #21)

One more thing that came up while moving over the localization stuff: Since we want the header to be correct for each component (e.g., "The [x] component has crashed"), is there a specific way we handle such cases? All I can think of is adding two localization strings (namely, "The" and "has crashed") so we can plug a componentName prop into the middle.

Yes, you can use a %S placeholder in the properties file (see webconsole.properties#362-365 ) and you can retrieve the string (with the replaced placeholder) using getFormatStr (devtools/shared/l10n.js#112-114 , e.g. : :LearnMoreLink.js#54 )

(In reply to Cody Welsh from comment #19)

Awesome, just wanted to make sure!

Secondary question: How do we want to handle all of the different CSS declarations for the different areas this component will be used in? We could just have the base AppErrorBoundary.css with specific declarations for everything, for example:

.network-monitor .app-error-panel {...}
...
.sidebar-error-boundary .app-error-panel {...}
...
.webconsole .app-error-panel {...}

I guess we could use the Network tab's error boundary CSS as a base, and declare specific styles as necessary for these others to also work. There are a bunch of other options here, though.

Mh, not sure for now. I think I'd rather have a className prop on the AppErrorBoundary component, that would be put alongside app-error-panel, so in css we could do

.app-error-panel.webconsole-app-error { 

or something along those lines.

Comment 23

[Cody Welsh]

Attachment: Bug 1675762 - Use AppErrorBoundary component in the Console panel. r=nchevobbe,honza

Add error boundaries to the Web Console, the JS Terminal, and the Web
Console sidebar, along with appropriate CSS and localization refactoring
necessary to make them all work. Also abstract the Net Monitor error
boundary so that these four areas refer to the same shared code.

Comment 24

[Hubert Boma Manilla (:bomsy)]

This should be a lot easier since Bug 1731460 landed

Comment 25

[Hubert Boma Manilla (:bomsy)]

Hey Cody,
As no activity for a while, i'll reassign.
Thanks so much

Comment 26

[Claudia Batista [:claubatista]]

Attachment: Bug 1675762 - [devtools] Use AppErrorBoundary component in the Console panel r=bomsy,nchevobbe

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5072d25cf6cc
[devtools] Use AppErrorBoundary component in the Console panel r=bomsy,nchevobbe

Comment 28

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5072d25cf6cc

Comment 29

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/5072d25cf6cc