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.

Sign up for GitHub

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

Jump to bottom

[Gutenberg] Add error boundary components and exception logging #22655

Merged
merged 26 commits into from
Mar 11, 2024
Merged

[Gutenberg] Add error boundary components and exception logging #22655

merged 26 commits into from
Mar 11, 2024

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Feb 21, 2024 •
edited
Loading

Related PRs:

This PR enables the error boundary at the editor and block level. It also connects Gutenberg Mobile to the Crash logging service in order to log JavaScript exceptions.

Additionally, it uploads to Sentry the JavaScript source map included in the GBM XCFramework.

To test:
Follow testing instructions from wordpress-mobile/gutenberg-mobile#6655.

Regression Notes

  1. Potential unintended areas of impact
    None on the user-facing side as this PR mainly implements a new RN bridge function. However, it also adds a step in the build process that if fails might disrupt the release process.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    I manually tested the build process for alpha builds. I didn't cover beta and app store builds, but the only error I foresee that it might introduce is tagging the source map files with a wrong release version. We'll work with Apps Infra team closely in the release where these changes are introduced to validate the source map files are properly uploaded.

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing checklist:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@fluiddot fluiddot added the Gutenberg Editing and display of Gutenberg blocks. label Feb 21, 2024
This was referenced Feb 21, 2024
Merged
Merged
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 21, 2024 •
edited
Loading

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr22655-eff477f
Version24.4
Bundle IDorg.wordpress.alpha
Commiteff477f
App Center BuildWPiOS - One-Offs #9120
 Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Feb 21, 2024 •
edited
Loading

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr22655-eff477f
Version24.4
Bundle IDcom.jetpack.alpha
Commiteff477f
App Center Buildjetpack-installable-builds #8163
 Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

fluiddot
fluiddot commented Feb 26, 2024
View reviewed changes
fastlane/lanes/build.rb
Comment on lines +178 to +183
upload_gutenberg_sourcemaps(
sentry_project_slug: SENTRY_PROJECT_SLUG_WORDPRESS,
release_version: release_version_current,
build_version: build_code_current,
app_identifier: WORDPRESS_BUNDLE_IDENTIFIER
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This source map will be used for the App Store builds (beta and production) of the WordPress app.

fastlane/lanes/build.rb
Comment on lines +231 to +236
upload_gutenberg_sourcemaps(
sentry_project_slug: SENTRY_PROJECT_SLUG_JETPACK,
release_version: release_version_current,
build_version: build_code_current,
app_identifier: JETPACK_BUNDLE_IDENTIFIER
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This source map will be used for the App Store builds (beta and production) of the Jetpack app.

fastlane/lanes/build.rb
Comment on lines +289 to +294
upload_gutenberg_sourcemaps(
sentry_project_slug: SENTRY_PROJECT_SLUG_WORDPRESS,
release_version: release_version_current_internal,
build_version: build_code_current_internal,
app_identifier: 'org.wordpress.internal'
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This source map will be used for the internal builds of the WordPress app. As far as I checked, this version is seems no longer used.

fastlane/lanes/build.rb Outdated
Comment on lines 421 to 426
upload_gutenberg_sourcemaps(
sentry_project_slug: sentry_project_slug,
release_version: release_version_current,
build_version: build_number,
app_identifier: app_identifier
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This source map will be used for the alpha builds (i.e. installable builds in PRs). The app is determined by the app_identifier parameter.

fastlane/lanes/build.rb Outdated
Comment on lines 486 to 487
# The bundle and source map files are the same for all architectures.
gutenberg_bundle = File.join(PROJECT_ROOT_FOLDER, "Pods/Gutenberg/Frameworks/Gutenberg.xcframework/ios-arm64/Gutenberg.framework")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As specified in the inline comment, the bundle and source map files are included for each architecture. However, both files are the same, as the JavaScript code isn't affected by the architecture. Hence, we can use the files from any of the architectures.

Scripts/BuildPhases/CopyGutenbergJS.sh
@@ -13,6 +13,7 @@ DEST="$CONFIGURATION_BUILD_DIR/$UNLOCALIZED_RESOURCES_FOLDER_PATH"
# Update the matching .inputs.xcfilelist when changing these
XCFRAMEWORK_BUNDLE_ROOT="$PODS_XCFRAMEWORKS_BUILD_DIR/Gutenberg/Gutenberg.framework"
PODS_BUNDLE_ROOT="$PODS_ROOT/Gutenberg/bundle/ios"
LOCAL_BUNDLE="$PODS_ROOT/../../gutenberg-mobile/bundle/ios"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When testing a production version of the JS bundle in local builds, I noticed we don't have a quick way to use a local bundle. For this reason, I added a third option to fetch the bundle by pointing to a local path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The option to fetch the bundle form $PODS_ROOT/Gutenberg/bundle/ios seems to be deprecated as the library is provided with the XCFramework. We could remove it.

@fluiddot fluiddot changed the title [Gutenberg] Add error boundary components and exception reporting [Gutenberg] Add error boundary components and exception logging Feb 28, 2024
@fluiddot fluiddot marked this pull request as ready for review February 28, 2024 12:38
@fluiddot fluiddot requested a review from jhnstn February 28, 2024 12:38
@fluiddot fluiddot mentioned this pull request Mar 1, 2024
Merged
13 tasks
jhnstn
jhnstn approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@jhnstn jhnstn left a comment

Choose a reason for hiding this comment

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

The test build worked from wordpress-mobile/gutenberg-mobile#6654) worked as expected.

Looks like there are some linting issues but otherwise looks good to go.
Nice work @fluiddot !

@fluiddot
Copy link
Contributor Author

fluiddot commented Mar 8, 2024

Looks like there are some linting issues but otherwise looks good to go.

The only remaining dangermattic error is the Tracks reference to a branch. Once Automattic/Automattic-Tracks-iOS#280 is merged, I'll update it with the release tag.

@fluiddot fluiddot added this to the 24.5 milestone Mar 11, 2024
@fluiddot fluiddot enabled auto-merge March 11, 2024 18:08
@fluiddot fluiddot merged commit 6b28a9d into trunk Mar 11, 2024
25 checks passed
@fluiddot fluiddot deleted the rnmobile/add/log-exception-to-crash-logging branch March 11, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dangermattic dangermattic dangermattic left review comments

@jhnstn jhnstn jhnstn approved these changes

Assignees
No one assigned
Labels
Gutenberg Editing and display of Gutenberg blocks. Observability
Projects
None yet
Milestone
24.5
Development

Successfully merging this pull request may close these issues.
4 participants
@fluiddot @wpmobilebot @jhnstn @dangermattic