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

[RNMobile] Add error boundry handling for Android #59385

Merged

Conversation

jhnstn
Copy link
Contributor

@jhnstn jhnstn commented Feb 26, 2024

What?

Adds Android support to #59221

Why?

How?

Testing Instructions

See #59221 for testing

Testing Instructions for Keyboard

Screenshots or screencast

@@ -4,7 +4,6 @@

import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.bridge.WritableMap;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe Android Studio removed this when it linted the file.

mGutenbergBridgeJS2Parent.logException(exception, logExceptionCallback);
}

private LogExceptionCallback onLogExceptionCallback(final Callback jsCallback) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm following the same encapsulation used for other callbacks in the bridge, such as ConnectionStatusCallback.

Copy link

Flaky tests detected in a16c50f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8056529535
📝 Reported issues:

@jhnstn jhnstn added Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) Mobile App - Automation Label used to initiate Mobile App PR Automation labels Mar 1, 2024
Copy link

github-actions bot commented Mar 1, 2024

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

  • Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
  • Labels found: Mobile App - i.e. Android or iOS, Mobile App - Automation.

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

@jhnstn I took a brief look at the PR and shared some initial comments related to parsing the exception. Feel free to omit them in case you are already aware.

@jhnstn jhnstn marked this pull request as ready for review March 6, 2024 21:12
Copy link

github-actions bot commented Mar 6, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: jhnstn <jsnjohnston@git.wordpress.org>
Co-authored-by: fluiddot <carlosgprim@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@fluiddot
Copy link
Contributor

fluiddot commented Mar 7, 2024

@jhnstn the CI job React Native E2E Tests (Android) failed with the following error:

file:///Users/runner/work/gutenberg/gutenberg/packages/react-native-editor/android/app/src/main/java/com/gutenberg/MainApplication.kt:88:84
Object is not abstract and does not implement abstract member public abstract fun logException(p0: GutenbergJsException!, p1: GutenbergBridgeJS2Parent.LogExceptionCallback!):
Unit defined in org.wordpress.mobile.ReactNativeGutenbergBridge.GutenbergBridgeJS2Parent

I think we are missing implementing LogExceptionCallback in the demo app.

UPDATE: Fixed in f3211da.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

LGTM 🎊 !

@fluiddot fluiddot merged commit 49f32e7 into rnmobile/add/error-boundary Mar 7, 2024
48 of 57 checks passed
@fluiddot fluiddot deleted the rnmobile/update/error-boundary/android branch March 7, 2024 15:38
fluiddot added a commit that referenced this pull request Mar 11, 2024
* 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 (#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>
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
Labels
Mobile App - Automation Label used to initiate Mobile App PR Automation Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants