webui: Adding report for JS bugs #5225

adamkankovsky · 2023-10-03T11:41:19Z

Modification of the existing bug report system and addition of a report option for critical bugs related to JS.

Based on commit: e242c12

Report looks like this:

Bugzilla:

KKoukiou · 2023-10-04T07:14:27Z

ui/webui/src/components/Error.jsx

+                    {exceptionNamePrefix + exception.message}
+                </Text>
+            </TextContent>
+            {exception.stack &&


I know this was cherry-picked from the commit that I wrote, but as said on slack, I think we do not need to show the stracktrace inside the error modal.
The reason we show the log from journal is to allow the user to edit it so as to remove sensitive information like passwords etc.

So I think it's enough to store the stackstrace into a file, like webui-js-stacktrace.txt, and that we inform the user about the existence of it.

The design can be discussed with @garrett

I'll definitely edit it again, I honestly didn't really understand the messages I have to do here, so I at least got the existing solution to work in all cases. Now I get it and I'll edit it

I agree with @KKoukiou about not showing the stacktrace.

And we wouldn't even show the log, except that it might have some user identifiable parts (although even this is somewhat questionable, as it's on a live system without user data... I suppose a created user or IP address might be shown? I can't think of much else that would be user-identifiable in the logs.)

Ideally, we'd just show a basic summary. People encountering errors can't really do anything about them for the most part.

KKoukiou · 2023-10-04T07:14:56Z

ui/webui/src/components/app.jsx

+
+        // Listen on JS errors
+        window.onerror = (err, url, line, col, errObj) => {
+            setOops(errObj);


Please remove the console.error, this was used for debugging.

adamkankovsky · 2023-10-05T10:03:55Z

@garrett If it would be possible, I would like to include in this error reporting window instead of the stacktrace block, just a short description of the error and a button to download the stacktrace in txt format, because as @KKoukiou said, there is no need to show the stacktrace unlike the log. This is of course just a sketch, it may turn out differently

garrett · 2023-10-05T10:04:54Z

What can someone do with a stack trace, if they're not an Anaconda developer?

(They could report it, but shouldn't that already be handled when they click the button to report it?)

adamkankovsky · 2023-10-05T10:06:00Z

What can someone do with a stack trace, if they're not an Anaconda developer?

They can add the file to the bugzilla for us.

garrett · 2023-10-05T10:15:22Z

They can add the file to the bugzilla for us.

There's a send button. Why not send that over?

Why make someone download a file somewhere, then go to the issue, and attach it? Worse yet, they may need to reboot into a working OS or use another computer... and then somehow find the URL for the bug. And this is even assuming that they made an account and signed in with it to report the bug in the first place. This is an extremely high barrier to actually do anything with a backtrace.

It needs to be attached to the report when clicking the button. Anything else is madness, and people will generally not do this (nor even be able to).

adamkankovsky · 2023-10-05T12:25:02Z

It

This would of course be the ideal situation if pressing the button would automatically pass everything to the bugzilla, since I talked with Radek, the log is added to the bugzilla later using a log file that is taken directly from the device, because the log is in most cases larger than the maximum message size in the bugzilla.
I see a problem here, for example with remote access, where you need to somehow get the log from the device and where it might be better to be able to download the log directly.
As for the js stacktrace, it should fit into the maximum message size.

rvykydal · 2023-10-10T11:15:01Z

I'd suggest to:

handle remote access challenges with attaching log from installer's filesystem separately
add just some lightwieight information to "Error detail", like "User interface error: stepIds.error is not a function." (no stacktrace displayed)
add the stacktrace to the BZ pre-filled BZ report "Details" (done via URL, it has limit ~ 8kb). Similar to

anaconda/ui/webui/src/components/Error.jsx

Line 185 in 553cdb1

newUrl.searchParams.append(

.

ui/webui/src/components/Error.jsx

ui/webui/src/components/app.jsx

rvykydal

I think we should keep the backdrop behind the dialog.

rvykydal

Looks good to me, thank you!
I just see not ideal formatting while trying it in firefox, but that is something I wouldn't block this PR on, I think we can investigate / solve it in a separate PR:

KKoukiou

Please test this with automation tests.

KKoukiou · 2023-10-12T15:55:28Z

ui/webui/src/components/Error.jsx

    );
    newUrl.searchParams.append(
        "comment",
-        "Installer WebUI Critical Error:\n" + context + exception.name + ": " + exception.message
+        "Installer WebUI Critical Error:\n" + context + name + exception.message + "\n\n StackTrace: " + exception.stack


For backend errors , the StrackTrace is not going to be there. Maybe remove this from the bug report?

KKoukiou · 2023-10-12T15:55:56Z

ui/webui/src/components/Error.jsx

@@ -228,7 +229,6 @@ export const CriticalError = ({ exception, isBootIso, isConnected, reportLinkURL
          buttons={[quitButton(isBootIso)]}
          isConnected={isConnected}
        />


This is an unrelated change, please make sure to keep your PRs clean from unrelated changes.

KKoukiou · 2023-10-12T15:55:59Z

ui/webui/src/components/Error.jsx

@@ -93,7 +93,7 @@ export const BZReportModal = ({
    detailsLabel,
    detailsContent,
    buttons,
-    isConnected
+    isConnected,


This is an unrelated change, please make sure to keep your PRs clean from unrelated changes.

Sorry, I dont clean it after my changes.

KKoukiou · 2023-10-12T15:56:39Z

ui/webui/src/components/app.jsx

+        // Listen on JS errors
+        window.onerror = (err, url, line, col, errObj) => {
+            setJsEroor(errObj);
+            err = null;


Where do you use the err? Why you set it to null?

This is probably not the ideal solution, but the parameters here are necessary for the function to work properly, but lint will display an error if I don't work/maintain the variable in any way, so this seemed like the least painful solution if we don't want to write it out.

Ah for that the common way to approach is to use underscore instead of the variable name so (_, _, _, _, errObj)

KKoukiou · 2023-10-12T15:58:30Z

ui/webui/src/components/app.jsx

-                          onAddErrorNotification={onAddErrorNotification}
+            {(criticalError || jsError) &&
+                <CriticalError
+                  exception={{ ...criticalError, message: jsError?.message || criticalError?.message, stack: jsError?.stack }}


THis is fine as first go probably, but I feel we might need to expect backend and front end crashing in parallel, so we need to report both.

KKoukiou · 2023-10-12T16:00:09Z

ui/webui/src/components/app.jsx

+                  isBootIso={isBootIso}
+                  isConnected={state.network.connected}
+                  reportLinkURL={bzReportURL} />}
+            {!jsError &&


Hm, I think it's fine to show the page in the background if an error occured. It's blurred anyhow - and the user can't get out of the CriticalError modal without quiting the installer eventually.

Unfortunately, the page must be hidden during a JS error where the error occurs in order to display the modal at all. If the js error is still running it is no longer possible to display any JS code.

KKoukiou

Please add a test (always :)) You can test raising an exception from the UI with:

with self.assertRaisesRegex(RuntimeError, "TypeError:.*undefined"):

KKoukiou · 2023-10-16T15:36:57Z

ui/webui/src/components/Error.jsx

@@ -177,14 +177,18 @@ export const BZReportModal = ({

 const addExceptionDataToReportURL = (url, exception) => {
    const newUrl = new URL(url);
+    const backendMessage = exception.backendMessage ? exception.backendMessage + (exception.jsMessage ? " " : "") : "";


You will probaly need new lines between then backend, jsMessage and stackTrace. Did you try it but I am not sure they are included in the errors (the new lines)

Can you please verify?

I checked and there are no newlines at the end of the reports. I added them to the url.

KKoukiou

Please add the screenshot and rebase.

KKoukiou · 2023-10-17T15:17:55Z

ui/webui/test/check-basic

+            b.wait_visible("#critical-error-bz-report-modal.pf-c-modal-box")
+            self._testLogReview(b, m, "critical-error")
+
+            b.assert_pixels(


There was not screenshot generated or added by you. I think it's because the class it wrong.

ui/webui/test/check-basic

KKoukiou · 2023-10-18T09:58:01Z

/kickstart-test --waive webui only

github-actions bot added webui f40 labels Oct 3, 2023

adamkankovsky force-pushed the webui-js-error-notify branch 2 times, most recently from 14eebfe to 063f4a6 Compare October 3, 2023 11:46

adamkankovsky marked this pull request as draft October 3, 2023 11:47

adamkankovsky force-pushed the webui-js-error-notify branch 3 times, most recently from 36d1c84 to cfc20ec Compare October 3, 2023 12:06

adamkankovsky requested review from rvykydal and KKoukiou October 3, 2023 12:07

adamkankovsky marked this pull request as ready for review October 3, 2023 12:29

adamkankovsky force-pushed the webui-js-error-notify branch from cfc20ec to 5feef0f Compare October 3, 2023 12:38

KKoukiou requested changes Oct 4, 2023

View reviewed changes

adamkankovsky force-pushed the webui-js-error-notify branch from 5feef0f to e1cc8b2 Compare October 10, 2023 18:38

rvykydal reviewed Oct 11, 2023

View reviewed changes

ui/webui/src/components/Error.jsx Outdated Show resolved Hide resolved

rvykydal reviewed Oct 11, 2023

View reviewed changes

ui/webui/src/components/Error.jsx Outdated Show resolved Hide resolved

rvykydal reviewed Oct 11, 2023

View reviewed changes

ui/webui/src/components/app.jsx Outdated Show resolved Hide resolved

rvykydal requested changes Oct 11, 2023

View reviewed changes

adamkankovsky force-pushed the webui-js-error-notify branch 2 times, most recently from de9ed41 to 06bb035 Compare October 11, 2023 16:05

adamkankovsky requested review from rvykydal and KKoukiou October 11, 2023 16:05

rvykydal approved these changes Oct 12, 2023

View reviewed changes

KKoukiou reviewed Oct 12, 2023

View reviewed changes

adamkankovsky force-pushed the webui-js-error-notify branch 2 times, most recently from 64da690 to f35c6ec Compare October 16, 2023 14:44

adamkankovsky marked this pull request as draft October 16, 2023 14:45

adamkankovsky force-pushed the webui-js-error-notify branch 2 times, most recently from 155da1f to f820f12 Compare October 16, 2023 15:09

KKoukiou reviewed Oct 16, 2023

View reviewed changes

adamkankovsky force-pushed the webui-js-error-notify branch 2 times, most recently from 9212db0 to 9a853c0 Compare October 17, 2023 14:36

KKoukiou reviewed Oct 17, 2023

View reviewed changes

adamkankovsky force-pushed the webui-js-error-notify branch 4 times, most recently from 39ec4ee to 8e3eebd Compare October 17, 2023 15:33

adamkankovsky marked this pull request as ready for review October 17, 2023 15:35

KKoukiou requested changes Oct 18, 2023

View reviewed changes

ui/webui/test/check-basic Outdated Show resolved Hide resolved

KKoukiou force-pushed the webui-js-error-notify branch from 8e3eebd to da80325 Compare October 18, 2023 08:49

webui: add report for JS bugs + test

b832747

KKoukiou force-pushed the webui-js-error-notify branch from da80325 to b832747 Compare October 18, 2023 08:49

KKoukiou merged commit f8e3b9d into rhinstaller:master Oct 18, 2023
17 checks passed

adamkankovsky mentioned this pull request Oct 18, 2023

Webui: Fix for adding report for JS bugs #5264

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webui: Adding report for JS bugs #5225

webui: Adding report for JS bugs #5225

adamkankovsky commented Oct 3, 2023 •

edited

KKoukiou Oct 4, 2023

adamkankovsky Oct 4, 2023

garrett Oct 5, 2023 •

edited

KKoukiou Oct 4, 2023

adamkankovsky commented Oct 5, 2023

garrett commented Oct 5, 2023 •

edited

adamkankovsky commented Oct 5, 2023

garrett commented Oct 5, 2023

adamkankovsky commented Oct 5, 2023 •

edited

rvykydal commented Oct 10, 2023

rvykydal left a comment

rvykydal left a comment

KKoukiou left a comment

KKoukiou Oct 12, 2023

KKoukiou Oct 12, 2023

KKoukiou Oct 12, 2023

adamkankovsky Oct 12, 2023

KKoukiou Oct 12, 2023

adamkankovsky Oct 12, 2023

KKoukiou Oct 12, 2023

KKoukiou Oct 12, 2023

KKoukiou Oct 12, 2023

adamkankovsky Oct 12, 2023 •

edited

KKoukiou left a comment

KKoukiou Oct 16, 2023

adamkankovsky Oct 17, 2023

KKoukiou left a comment

KKoukiou Oct 17, 2023

KKoukiou commented Oct 18, 2023

webui: Adding report for JS bugs #5225

webui: Adding report for JS bugs #5225

Conversation

adamkankovsky commented Oct 3, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrett Oct 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkankovsky commented Oct 5, 2023

garrett commented Oct 5, 2023 • edited

adamkankovsky commented Oct 5, 2023

garrett commented Oct 5, 2023

adamkankovsky commented Oct 5, 2023 • edited

rvykydal commented Oct 10, 2023

rvykydal left a comment

Choose a reason for hiding this comment

rvykydal left a comment

Choose a reason for hiding this comment

KKoukiou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamkankovsky Oct 12, 2023 • edited

Choose a reason for hiding this comment

KKoukiou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKoukiou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KKoukiou commented Oct 18, 2023

adamkankovsky commented Oct 3, 2023 •

edited

garrett Oct 5, 2023 •

edited

garrett commented Oct 5, 2023 •

edited

adamkankovsky commented Oct 5, 2023 •

edited

adamkankovsky Oct 12, 2023 •

edited