Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update message when WASM returns an error and allow user to restart the server #753

Merged
merged 9 commits into from
Dec 20, 2024

Conversation

sejas
Copy link
Member

@sejas sejas commented Dec 19, 2024

Related issues

Proposed Changes

  • Reject promise when the server dies when starting a server, so it can be started again by the user.
  • Change the message displayed to the user when we get the "unreachable" WASM instruction executed

Before this PR, when a site crashed, it got unresponsive, and users couldn't re-try starting it again.
The reason is that the server child process failed and didn't resolve or rejected a promise when stopping the site, which let the main thread waiting for messages. This was fixed on : a4afa34

Testing Instructions

  • Download a backup with a plugin that fails already activated. There is an example in the issue.
  • Run npm start
  • Create a blank site
  • Go to the import tab
  • Drag and drop the backup file
  • Wait until the import succeeds
  • Observe the site tries starting
  • Observe a popup error saying : "[...] Please try disabling plugins and themes that might be causing the issue."
  • Close the popup, wait a couple of seconds
  • Click on start again
  • Observe the same popup

Before

Screenshot 2024-12-19 at 16 07 03

After

Screenshot 2024-12-20 at 12 16 40

WEFING.gif

Pre-merge Checklist

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

@sejas sejas self-assigned this Dec 19, 2024
@sejas sejas requested a review from a team December 19, 2024 16:27
error instanceof Error &&
error.message.includes( '"unreachable" WASM instruction executed' )
) {
throw new Error( 'Please try disabling plugins and themes that might be causing the issue.' );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could translate the error message here with __, but we probably prefer to log, and capture in Sentry all the messages in English.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the Sentry call here, too, so we would log it before we rethrow the exception?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wojtekn , thanks for the suggestion. It makes sense. I've moved it here:

9c301a2

try {
await server.start();
} catch ( error ) {
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. In the future, we could handle other exceptions there with custom messages, or maybe remove the condition and use unified message for all case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I thought about overwriting all the errors, but I prefer to make the distinction in case we found any other error now or in the future after a dependency update.

@sejas sejas requested a review from wojtekn December 20, 2024 12:53
@wojtekn wojtekn merged commit ba4dd01 into trunk Dec 20, 2024
6 checks passed
@wojtekn wojtekn deleted the fix/playground-failing-start-site branch December 20, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants