Stop showing the component stack in case of an error, and show a more… by julienw · Pull Request #4526 · firefox-devtools/profiler

julienw · 2023-03-13T13:38:08Z

… helpful message

We get too many reports with useless stacks. So now I removed the stack output and instead advise the user to look at the devtools web console. I also provided a button for an easier access to github.

I also made it localized when possible.

The localization isn't perfect, because we concatenate strings instead of providing them in their entirety. The reason is that I didn't want to spend too much time on it, and I thought that because this is an error component that the users shouldn't see much that would be good enough. I think it's better now, thanks for your feedback.

julienw · 2023-03-13T13:42:31Z

                {this.props.message}
+                {errorString ? ` (${errorString})` : null}.{' '}
+                {this.props.reportExplanationMessage}


@flodolo
I know that the localization isn't perfect, because we concatenate strings instead of providing them in their entirety. The reason is that I didn't want to spend too much time on it, and I thought that because this is an error component that the users shouldn't see much that would be good enough. I hope that's good enough for you!

Can you help me understand where the Error: part come from?

I'm really not a fan of having it added as part of the first sentence. Can we keep using Details--error-boundary-message, and insert (concatenate) a fully-formed separate sentence Error: foo. instead? Or maybe put that at the very end, between the localized paragraph and the button?

This comes from calling toString on the error object. I this case the error object was a new Error("foo") but usually it's an Error with more context (hopefully).

I agree with @flodolo's comment. I don't think we need to change the Details--error-boundary-message string here. We can separate the message and error itself to 2 different lines/paragraphs (without putting the error in a paranthesis). I think it's also easier to read for the users in case the error message is very long.

Thanks for the feedback, I think this is indeed better now:

Telll me what you think :-)

julienw · 2023-03-13T13:43:20Z

+              id="Root--error-boundary-message"
+              attrs={{ message: true }}
+            >
+              <LocalizedErrorBoundary message="Uh oh, some error happened in profiler.firefox.com">


Here I added another one so that the non localized version would be only for errors in AppLocalizationProvider.

julienw · 2023-03-13T13:43:44Z

  });
 });
+
+describe('app/LocalizedErrorBoundary', function () {


I just added a small test for this new component.

codecov · 2023-03-13T13:54:43Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.15 ⚠️

Comparison is base (0300bbe) 88.63% compared to head (582403c) 88.48%.

❗ Current head 582403c differs from pull request most recent head 884a6b4. Consider uploading reports for the commit 884a6b4 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4526      +/-   ##
==========================================
- Coverage   88.63%   88.48%   -0.15%     
==========================================
  Files         293      287       -6     
  Lines       25983    25803     -180     
  Branches     7000     6940      -60     
==========================================
- Hits        23029    22832     -197     
- Misses       2748     2765      +17     
  Partials      206      206

Impacted Files	Coverage Δ
src/components/app/Details.js	`85.00% <ø> (ø)`
src/components/app/Root.js	`100.00% <ø> (ø)`
src/components/app/ErrorBoundary.js	`96.00% <100.00%> (-0.30%)`	⬇️

... and 20 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

canova · 2023-03-14T14:08:56Z

-                type="button"
-                onClick={this._toggleErrorDetails}
-                aria-expanded={showDetails ? 'true' : 'false'}
+                href="https://github.com/firefox-devtools/profiler/issues/new?title=An%20error%20occurred%20in%20Firefox%20Profiler"


I think we can even populate the content of the bug to include the error message etc. But we can do that later.

canova · 2023-03-14T14:17:39Z

+Details--error-boundary-message2 =
+    .message = Uh oh, some unknown error happened in this panel


I think we can leave this message as is, with the dot at the end if we move the error to a line below.

canova · 2023-03-14T14:20:59Z

                {this.props.message}
+                {errorString ? ` (${errorString})` : null}.{' '}
+                {this.props.reportExplanationMessage}


I agree with @flodolo's comment. I don't think we need to change the Details--error-boundary-message string here. We can separate the message and error itself to 2 different lines/paragraphs (without putting the error in a paranthesis). I think it's also easier to read for the users in case the error message is very long.

… helpful message

julienw · 2023-03-15T14:03:30Z

-  display: flex;
-  align-items: center;
-


I looked closely at our example page, and I think that this wasn't useful after all, and this was in the way when I tried to add more than 1 child inside this element. So I removed them. But we will need to take care at regressions in some of our notification bars.

julienw · 2023-03-15T14:03:55Z

  width: 100%;
-  max-width: 600px;
-  max-height: calc(100%);
+  max-width: 800px;


I gave some more space to the notice.

julienw · 2023-03-15T14:04:10Z

-  max-width: 600px;
-  max-height: calc(100%);
+  max-width: 800px;
+  max-height: 100%;


and remove the calc which didn't seem useful (?).

julienw · 2023-03-15T14:04:23Z

-  flex: none;
-}
-
-.appErrorBoundaryDetails {


I forgot to remove those in the first version of the patch.

julienw · 2023-03-15T14:06:49Z

-     time for the fade out. */
-  transition: opacity 150ms, visibility 0s 150ms;
-  visibility: hidden;
+.appErrorBoundaryInnerText > p {


I wanted to use p which is semantically better, but it comes with default margin... One day we'll probably want to include some version (or our version) of normalize.css...

Yeah, it would be good to have a normalize.css.

julienw · 2023-03-15T14:07:24Z

    Please report this issue to the developers, including the full
    error as displayed in the Developer Tools’ Web Console.

+# This is used in a call to action button, displayed inside the error box.


I added some more comments here as well.

julienw · 2023-03-15T14:11:08Z

oops sorry @flodolo, I missed that your r+ was just given now and I resetted it by mistake :-)

flodolo

LGTM

julienw · 2023-03-15T14:12:46Z

Hey @canova, I added all new changes to a separate commit :-) Please tell me what you think!

Because I changed some photon styles, I give you the direct link to the deploy preview for the photon example page: https://deploy-preview-4526--perf-html.netlify.app/photon/.

canova

Looks good to me, thanks!

canova · 2023-03-16T09:44:24Z

-     time for the fade out. */
-  transition: opacity 150ms, visibility 0s 150ms;
-  visibility: hidden;
+.appErrorBoundaryInnerText > p {


Yeah, it would be good to have a normalize.css.

julienw requested a review from a team as a code owner March 13, 2023 13:38

julienw commented Mar 13, 2023

View reviewed changes

julienw requested a review from canova March 13, 2023 13:47

canova reviewed Mar 14, 2023

View reviewed changes

julienw added 2 commits March 15, 2023 14:34

Stop showing the component stack in case of an error, and show a more…

3d70860

… helpful message

Fix review comments

91259a4

julienw force-pushed the stop-output-traces-in-error branch from 582403c to 91259a4 Compare March 15, 2023 13:58

julienw commented Mar 15, 2023

View reviewed changes

flodolo approved these changes Mar 15, 2023

View reviewed changes

julienw commented Mar 15, 2023

View reviewed changes

julienw requested review from canova and flodolo March 15, 2023 14:09

flodolo approved these changes Mar 15, 2023

View reviewed changes

canova approved these changes Mar 16, 2023

View reviewed changes

Merge branch 'main' into stop-output-traces-in-error

884a6b4

julienw enabled auto-merge (squash) March 16, 2023 10:33

julienw merged commit f9a5b4e into firefox-devtools:main Mar 16, 2023

julienw mentioned this pull request May 15, 2023

Fix the text in buttons not being centered properly #4620

Merged

		Details--error-boundary-message2 =
		.message = Uh oh, some unknown error happened in this panel

Conversation

julienw commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Mar 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julienw commented Mar 15, 2023

Uh oh!

flodolo left a comment

Choose a reason for hiding this comment

Uh oh!

julienw commented Mar 15, 2023

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

julienw commented Mar 13, 2023 •

edited

Loading

codecov Bot commented Mar 13, 2023 •

edited

Loading