Clean up error messages displayed in the error box #756

bcotrim · 2024-12-19T20:58:53Z

Related issues

Closes https://github.com/Automattic/dotcom-forge/issues/9624

Proposed Changes

Adds optional "Open Studio Logs" button to error dialogs
Clean up error cleanup and logging logic away from showErrorMessageBox

Testing Instructions

Prerequisites

Pull the changes from this branch
Start Studio with npm start

Test Scenarios

Error Dialog Display
- Trigger an error (e.g., rename a server folder while app is running and try to start that server)
- Verify the error dialog:
  - Shows message and error
  - Displays "Open Studio Logs" button when showOpenLogs is true
  - Shows "OK" button
Log File Access
- Click "Open Logs" button
- Verify log file opens with complete error details
Dialog Dismissal
- Click "OK" button
- Verify dialog closes properly

Expected Results

Error logging and message display responsibility have been moved away from showErrorMessageBox function
Developers are able to handle and parse error messages and choose what to display
Developers are able to offer the users the option to check logs when displaying error messages

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

src/ipc-handlers.ts

wojtekn · 2024-12-20T08:52:24Z

src/ipc-handlers.ts

 	);
-	await showMessageBox( event, {
+
+	console.error( filteredError );


Let's not make this method responsible of logging the error and leave it up for the caller.

Given #753, we could clean logic around filteredError and error in this method.

I think it would be preferable to display the error twice than none.
We could display some empty lines and a clear text in the log, like: "ERROR REPORTED:" or "ERROR OCCURRED:". This way the user and we would be much more sure about what error was the responsible of the alert.

Here is a possible implementation:

const errorMessage = ( error as Error )?.message; if ( errorMessage ) { console.error( '\n\n\n\n\n\nERROR REPORTED:\n' + errorMessage ); }

If we follow this path, we could rename the parameter name from error to errorToLog, so it's clear that the error will be logged in this function.

This is an example of how it would look:

I also see Wojtek's point of view from the perspective of a single responsibility, so open to other ideas.

Yes, let's remove the logging from this error and filteredError handling as those are solved outside the method scope.

Co-authored-by: Wojtek Naruniec <[email protected]>

sejas

Code looks good, and it works as expected.
Not approving it until we decide if we should log or not the error.
I also see a possibility to display the first line of characters of the error inside the alert, that way users could share a screenshot of Studio too. But it's not a strong opinion.

I wonder if there are errors where we show the alert and we don't log them into the file. Then in those cases the "Open Studio logs" button would be misleading.

I only tested it on Mac:

Clicking the Open Studio logs it opens the console app with the correct file almost at the bottom:

sejas · 2024-12-20T10:11:27Z

src/ipc-handlers.ts

 	);
-	await showMessageBox( event, {
+
+	console.error( filteredError );


I think it would be preferable to display the error twice than none.
We could display some empty lines and a clear text in the log, like: "ERROR REPORTED:" or "ERROR OCCURRED:". This way the user and we would be much more sure about what error was the responsible of the alert.

Here is a possible implementation:

const errorMessage = ( error as Error )?.message; if ( errorMessage ) { console.error( '\n\n\n\n\n\nERROR REPORTED:\n' + errorMessage ); }

If we follow this path, we could rename the parameter name from error to errorToLog, so it's clear that the error will be logged in this function.

This is an example of how it would look:

I also see Wojtek's point of view from the perspective of a single responsibility, so open to other ideas.

sejas · 2024-12-20T10:12:34Z

src/ipc-handlers.ts

-		detail: error ? `${ message }\n\nError: ${ filteredError }` : message,
-		buttons: [ __( 'OK' ) ],
+		detail: message,
+		buttons: [ __( 'Open Studio logs' ), __( 'OK' ) ],


Should we display the "Open Studio logs" buttons in all the cases? or just when we receive the "errorToLog" parameter?

Good point! We could use a param, e.g. showOpenLogs, that is set to false by default, and we can enable it per the case.

wojtekn · 2024-12-20T11:09:22Z

src/ipc-handlers.ts

+	const response = await showMessageBox( event, {
 		type: 'error',
 		message: title,
-		detail: error ? `${ message }\n\nError: ${ filteredError }` : message,


Let's bring it back so the specific error can still be appended, but let's drop the 'Error: '.

wojtekn · 2024-12-20T11:09:58Z

src/ipc-handlers.ts

 	);
-	await showMessageBox( event, {
+
+	console.error( filteredError );


Yes, let's remove the logging from this error and filteredError handling as those are solved outside the method scope.

wojtekn · 2024-12-20T11:10:46Z

src/ipc-handlers.ts

-		detail: error ? `${ message }\n\nError: ${ filteredError }` : message,
-		buttons: [ __( 'OK' ) ],
+		detail: message,
+		buttons: [ __( 'Open Studio logs' ), __( 'OK' ) ],


Good point! We could use a param, e.g. showOpenLogs, that is set to false by default, and we can enable it per the case.

wojtekn · 2024-12-20T14:23:56Z

@bcotrim my bad, I was wrong on filteredError. This should stay as it filters ugly addition added by IPC Handler. I reverted this part and merged trunk in.

wojtekn · 2024-12-20T14:27:05Z

src/hooks/use-site-details.tsx

 						"Please verify your site's local path directory contains the standard WordPress installation files and try again. If this problem persists, please contact support."
 					),
 					error,
+					showOpenLogs: true,


I'm enabling a new button for cases in which Studio can't start the site.

Will add more under https://github.com/Automattic/dotcom-forge/issues/10143

clean up error messages displayed in the error box

cc8e7b0

bcotrim requested a review from a team December 19, 2024 20:58

wojtekn reviewed Dec 20, 2024

View reviewed changes

update logs button

48be7f0

Co-authored-by: Wojtek Naruniec <[email protected]>

sejas reviewed Dec 20, 2024

View reviewed changes

wojtekn reviewed Dec 20, 2024

View reviewed changes

bcotrim and others added 3 commits December 20, 2024 12:58

adjust show open logs logic and remove logging

59be696

Merge branch 'trunk' into add/clean_error_messages

784b87d

Bring back logic that filters IPC handler wording

dbdcdab

Show logs button when Studio fails to start site

f3f87c1

wojtekn reviewed Dec 20, 2024

View reviewed changes

wojtekn approved these changes Dec 20, 2024

View reviewed changes

bcotrim added the Reviewer can merge label Dec 20, 2024

fix unit test

16d858f

bcotrim merged commit 6d67471 into trunk Dec 20, 2024
6 checks passed

bcotrim deleted the add/clean_error_messages branch December 20, 2024 14:55

Clean up error messages displayed in the error box #756

Clean up error messages displayed in the error box #756

Uh oh!

Conversation

bcotrim commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Prerequisites

Test Scenarios

Expected Results

Pre-merge Checklist

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!

sejas left a comment

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!

wojtekn commented Dec 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bcotrim commented Dec 19, 2024 •

edited

Loading