Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

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

Closed
wants to merge 9 commits into from

Conversation

mickmister
Copy link
Contributor

The PR this is based off of #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 #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

Screenshots

Screen.Recording.2022-09-17.at.10.28.09.AM.mov

Release Note

Prevent plugin components from crashing the entire web app

@mickmister mickmister added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Oct 28, 2022
@mattermod
Copy link
Contributor

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.

@AshishDhama
Copy link
Contributor

@mickmister Could you please resolve the conflicts

@mickmister
Copy link
Contributor Author

@AshishDhama CI issues fixed 👍

Copy link
Contributor

@AshishDhama AshishDhama left a comment

Choose a reason for hiding this comment

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

LGTM!

@AshishDhama AshishDhama removed the 2: Dev Review Requires review by a core commiter label Nov 28, 2022
@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Feb 14, 2023
@lieut-data
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3: QA Review Requires review by a QA tester release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants