mattermost-webapp icon indicating copy to clipboard operation
mattermost-webapp copied to clipboard

MM-47044 Wrap error boundary around plugin components that are not using `Pluggable` component

Open mickmister opened this issue 2 years ago • 1 comments

The PR this is based off of https://github.com/mattermost/mattermost-webapp/pull/11149 was closed due to automation when the target branch of that PR was closed, so this is being opened to replace that PR.

Summary

This PR is a continuation of https://github.com/mattermost/mattermost-webapp/pull/11148, which adds an error boundary to the Pluggable component. Some plugin components are currently not rendered through the Pluggable component. Some components are also stored on a different reducer than the one Pluggable is expecting to find them at state.plugins.components.

This PR makes it so the following cases have error boundaries added:

  • Plugin components that are in state.plugins.components, but are not using the Pluggable component
  • Plugin post types - uses its own reducer
  • Plugin post cards - uses its own reducer
  • Admin console custom settings - uses its own reducer

Each of these components ideally should use the Pluggable component, in order to have a single place to handle plugin-related kill switches etc. At the moment, this PR instead wraps the rendered components with the PluggableErrorBoundary. One solution to have the Pluggable used by all the components is to have the Pluggable component's props accept an arbitrary list of pluggable components, so it doesn't of requiring the components to be in a specific reducer, and can support custom filtering etc.

Another topic - I'm not sure how we can make it so new plugin component types that are introduced use the Pluggable component.

I'm also wanting to add try/catch blocks around plugin hooks that are function calls unrelated to rendering React components, to further increase the durability against plugin errors.

Ticket Link

https://mattermost.atlassian.net/browse/MM-47044

Related Pull Requests

  • https://github.com/mattermost/mattermost-webapp/pull/11148

Screenshots

https://user-images.githubusercontent.com/6913320/190862264-a4203888-62b0-4119-a1ee-a886c378ca37.mov

Release Note

Prevent plugin components from crashing the entire web app

mickmister avatar Oct 28 '22 03:10 mickmister

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

mattermod avatar Oct 28 '22 03:10 mattermod

@mickmister Could you please resolve the conflicts

AshishDhama avatar Nov 14 '22 04:11 AshishDhama

@AshishDhama CI issues fixed :+1:

mickmister avatar Nov 17 '22 19:11 mickmister

Heads up that as part of our efforts to move to a monorepo, we're closing out a number of older pull requests like this one to help streamline the effort.

If you'd like to preserve these changes -- even if you're not the original author! -- feel free to resubmit the pull request against the monorepo once it's ready. You can subscribe to mattermost-server-issue-22420 for status updates on this effort.

lieut-data avatar Mar 02 '23 21:03 lieut-data