[RNMobile] Add error boundary components and exception logging #59221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluiddot
added
[Type] Enhancement
2 tasks
|
Size Change: -230 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
fluiddot
commented
Feb 20, 2024
This was referenced Feb 21, 2024
[Gutenberg] Add error boundary components and exception logging
wordpress-mobile/WordPress-iOS#22655
Merged
The parameter `framesToPop` was dropped in RN version 0.62 (facebook/react-native@8bc02fd).
fluiddot
commented
Feb 26, 2024
| }; | ||
| } | ||
|
|
||
| static getDerivedStateFromError( error ) { |
There was a problem hiding this comment.
Here's more information about this static function: https://react.dev/reference/react/Component#static-getderivedstatefromerror
| return { error }; | ||
| } | ||
|
|
||
| componentDidCatch( error ) { |
There was a problem hiding this comment.
Here's more information about this component function: https://react.dev/reference/react/Component#componentdidcatch
Comment on lines
26
to
27
| error_boundary_level: 'block', | ||
| block_name: blockName, |
There was a problem hiding this comment.
As additional context to debug the exception, we'll attach at what level the exception happened and the block's name.
There was a problem hiding this comment.
As shared in inline comments, most of this logic is based on React Native and JavaScript Sentry SDKs.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @crazytonyli. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
fluiddot
changed the title
crazytonyli
reviewed
Feb 28, 2024
| } | ||
|
|
||
| self.delegate?.gutenbergDidRequestLogException(exception) { | ||
| callback([true]) |
There was a problem hiding this comment.
Considering the argument (a.k.a the wasSent argument in the JS function) is always true, what do you think removing it instead?
In reality, we can't really know if an even is actually being sent. We know it's received by Sentry SDK, but it's totally up to Sentry when it comes to actually sending the event to the server.
There was a problem hiding this comment.
It's true that wasSent won't tell if the event was received by Sentry. However, this parameter can be false if the rawException argument is malformed and can't be parsed to a GutenbergJSException and hance won't be sent to the Crash logging service. Maybe we should rename it to clarify the purpose of the parameter to wasProcessed or similar, WDYT?
gutenberg/packages/react-native-bridge/ios/RNReactNativeGutenbergBridge.swift
Lines 432 to 435 in c017e12
There was a problem hiding this comment.
Or maybe we could consider that wasSent is related to sending the exception to the Crash logging service, not specifically that was actually sent and processed by Sentry. WDYT?
There was a problem hiding this comment.
I must have missed the callback([false]) line above. The current code looks good to me, because there are different argument value used. 👍
There was a problem hiding this comment.
I'm not so sure we need to inform the JS side that the rawException is malformed. Right now, all it does is log the error, which seems we could do here in the bridge code.
I can see using a callback if the RN component will change given something failing down the call stack. The boundary component is already handling the error gracefully; everything else feels like "call and move on", much like how we call into the Sentry SDK without really knowing what happens next.
There was a problem hiding this comment.
I agree that the parameter returned in the callback could be dropped. However, we need the callback to ensure that unhandled exceptions are logged. Following this code, if we call defaultHandler right after calling the bridge function logException, the exception is not logged because the undhandled exception leads to crashing the app before the Sentry event is sent. This is the main reason why I introduced the callback.
There was a problem hiding this comment.
Following this code, if we call defaultHandler right after calling the bridge function logException,
Ah, okay. Thanks for pointing that out. I wasn't sure where the defaultHandler came from, but I see it's a ReactNative global, so we need to intercept that. Makes sense now.
There was a problem hiding this comment.
the undhandled exception leads to crashing the app
@fluiddot By "crashing the app", do you mean the iOS app, or the Gutenberg Editor? Out of curiosity, if an unexpected JS exception happens, it'd only affect the running JavaScriptCore VM, not the host app, right?
There was a problem hiding this comment.
The iOS app. By default, React Native causes a native crash for unhandled exceptions like other exceptions that might happen in the app.
It’s true that technically it only affects the JS runtime, but currently it behaves this way. In the future, we could change this behavior to closing the editor (i.e. a soft crash of the editor) instead of crashing and present post recovery options.
crazytonyli
reviewed
Feb 28, 2024
crazytonyli
reviewed
Mar 1, 2024
13 tasks
* Add logException to Android bridge * Add logException to Android demo app * Add GutenbergJsException data class * Update logException to use data class and return bool to callback * integrate exception handling in the glue code * remove past tense 'did' * Refactor exception processing * lint * Add `logException` bridge function in demo app * Update stacktrace parsing on Android --------- Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
|
@osullivanchris We'd appreciate your input regarding the error state when the editor encounters a non-recoverable exception (see attached screenshots to the PR). For reference, I used a similar approach as we have on the web. Thanks 🙇 ! |
4 tasks
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Mar 27, 2024
…ress#59221) * Add parse function to send exceptions over the RN bridge * Add RN bridge function to log exceptions to the host app * Add error boundary to blocks * Add error boundary at editor level * Log exceptions from error boundary components * Remove `in_app` stack trace parameter * Remove `getPopSize` The parameter `framesToPop` was dropped in RN version 0.62 (facebook/react-native@8bc02fd). * Simplify functions from `parseException` * Rename exception tag to `gutenberg_mobile_version` * Add `gutenbergDidRequestLogException` bridge function to demo app * Trigger callback upon sending JS exception * Format `RNReactNativeGutenbergBridge` * Update inline comments * Merge `reverseEntries` logic into `parseStacktrace` * Set second param of `parseException` (context and tags) as optional * Add unit tests of `parseException` * Add inline comment to `getReactNativeContext` * Add typing for JavaScript exception in bridge * Fix param type in `gutenbergDidRequestLogException` * Update `gutenbergDidRequestLogException` implementation of demo app * Rename `JSException` to avoid disambiguation with Crash Logging service * Add `actions` prop to `Warning` component * Allow extra styles in `Warning` component * Implement copy buttons in Error boundary component * Fix style import path in error boundary component * Change GutenbergJSException `function` param to non-optional * Rename JSException param to `message` * Rename GutenbergJSException param to `message` * Update `react-native-editor` changelog * Update unit tests of `parseException` * [RNMobile] Add error boundry handling for Android (WordPress#59385) * Add logException to Android bridge * Add logException to Android demo app * Add GutenbergJsException data class * Update logException to use data class and return bool to callback * integrate exception handling in the glue code * remove past tense 'did' * Refactor exception processing * lint * Add `logException` bridge function in demo app * Update stacktrace parsing on Android --------- Co-authored-by: Carlos Garcia <fluiddot@gmail.com> --------- Co-authored-by: Jason Johnston <jhnstn@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related PRs:
What?
Enables error boundary to prevent editor crashes at editor and block levels. Additionally, it implements a bridge function to log exceptions to host apps.
Why?
It will prevent the app from crashing from exceptions that occur in the editor, as well as provide detailed information of exceptions to the host apps.
How?
Testing Instructions
Follow testing instructions from wordpress-mobile/gutenberg-mobile#6655.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
Block-level error boundary
Editor-level error boundary
Web version:
