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

feat(1852): Implement sentry user report on error screen #27857

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

DDDDDanica
Copy link
Contributor

@DDDDDanica DDDDDanica commented Oct 15, 2024

Description

Sentry provides a feature to collect user feedback to users when an issue occurs. This could be a great help to fix Sentry issues which sometimes come without a lot of context and are difficult to reproduce.

Hence we use out-of-box solution from sentry to implement User Feedback Widget via Sentry.feedbackIntegration. You can find more technical details in this comment.

Design Figma link here.

What's expected in this PR:

  • Refactor original ui/pages/error/error.component.js to typescript, clean up / update language content, and improve the layout based on new design (see above Figma link) that would be consistent as mobile implementation
  • Add a new option in develop options to cause a page crash by remove one language file (for me was easiest way to trigger), which will bring us to error page
  • In new error page, we have 3 options:
  1. Describe what happened - open a form to sent a message to sentry
  2. Contact support - existing link to redirect to process.env.SUPPORT_REQUEST_LINK
  3. Try again - close the extension and allow user to open again
  • Convert ui/ducks/locale/locale.js to typescript and add related tests
  • Add e2e tests with POM pattern

This is the scenario for extension:

  • GIVEN a user has MM installed
  • AND Sentry is enabled (user enabled MetaMetrics)
  • WHEN an unhandled issue occurs in MM
  • THEN MM crashes
  • AND an event is sent to Sentry
  • AND user is given the possibility to describe what happened to him by filling a form
  • AND his feedback gets paired to the Sentry event once user presses the "submit" button at the bottom of the form
  • AND user is given more comprehensive error screen when it crashes

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/1852

Manual testing steps

  1. Set up sentry (https://github.com/MetaMask/metamask-extension/blob/develop/development/README.md)
  2. Add DSN to SENTRY_DSN_DEV in local env set up, and mark ENABLE_SETTINGS_PAGE_DEV_OPTIONS=true
  3. Run yarn webpack --watch --sentry
  4. Ensure Participate in MetaMetrics is opt in
  5. Click "develop options" in settings
  6. Click "Generate A Page Crash" button
  7. User is redirected to new error page
  8. Click Describe what happened can open a sentry feedback form, then in your sentry project you can find the submitted form within User Feedback section
  9. Click Contact support will redirect user to metamask support page
  10. Click Try again will close the extension and ready for reload

Screenshots/Recordings

Before

After

2024-10-16.13.09.02.mov

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@DDDDDanica DDDDDanica force-pushed the feature/1852-sentry-feedback branch 12 times, most recently from ead4ad9 to 6ff3574 Compare October 15, 2024 22:44
@DDDDDanica DDDDDanica changed the title Draft: feat(1852): Implement sentry user report on error screen feat(1852): Implement sentry user report on error screen Oct 15, 2024
@DDDDDanica DDDDDanica force-pushed the feature/1852-sentry-feedback branch 2 times, most recently from 354e60a to c9fcbc7 Compare October 15, 2024 23:57
@@ -177,7 +177,7 @@ function missingKeyError(
onError?.(error);
log.error(error);

if (process.env.IN_TEST) {
if (process.env.IN_TEST || process.env.ENABLE_SETTINGS_PAGE_DEV_OPTIONS) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this enable to trigger a crash from develop option

@@ -33,7 +33,7 @@ describe('Add snap account experimental settings @no-mmi', function (this: Suite
await settingsPage.goToExperimentalSettings();

const experimentalSettings = new ExperimentalSettings(driver);
await settingsPage.check_pageIsLoaded();
await experimentalSettings.check_pageIsLoaded();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo fixing

let isMounted = true; // For preventing memory leak

if (sentryButtonRef.current && isMounted) {
feedback.attachTo(sentryButtonRef.current); // Attach feedback widget to button
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bound trigger to button click

setCurrentLocale(currentLocale, {
...localeMessages,
// @ts-expect-error - remove a language string in this page to trigger a page crash
developerOptions: undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove a language file in current page to trigger crash

@@ -326,7 +326,7 @@ class SettingsPage extends PureComponent {
},
];

if (process.env.ENABLE_SETTINGS_PAGE_DEV_OPTIONS) {
if (process.env.ENABLE_SETTINGS_PAGE_DEV_OPTIONS || process.env.IN_TEST) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enable develop options in e2e test

@@ -395,7 +395,8 @@ class SettingsPage extends PureComponent {
/>
<Route exact path={SECURITY_ROUTE} component={SecurityTab} />
<Route exact path={EXPERIMENTAL_ROUTE} component={ExperimentalTab} />
{process.env.ENABLE_SETTINGS_PAGE_DEV_OPTIONS && (
{(process.env.ENABLE_SETTINGS_PAGE_DEV_OPTIONS ||
process.env.IN_TEST) && (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enable develop options in e2e test

@DDDDDanica DDDDDanica force-pushed the feature/1852-sentry-feedback branch 4 times, most recently from f0acd52 to e4489e9 Compare October 16, 2024 11:57
Copy link

sonarcloud bot commented Oct 16, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [e4489e9]
Page Load Metrics (1636 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31020311572316152
domContentLoaded14871894159310349
load15052067163612460
domInteractive24203403919
backgroundConnect8170453718
firstReactRender442081115024
getState5102252914
initialActions00000
loadScripts1104140911998440
setupStore11129303215
uiStartup169327091895225108

@metamaskbot
Copy link
Collaborator

Builds ready [ccdf22c]
Page Load Metrics (1674 ± 53 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15762086168111153
domContentLoaded15302075164711756
load15742084167411153
domInteractive24210544823
backgroundConnect86824199
firstReactRender43200783718
getState46112168
initialActions0643147
loadScripts11101667123611857
setupStore1174302512
uiStartup17222242186614569
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 3.21 KiB (0.04%)
  • common: 1.41 KiB (0.02%)

@DDDDDanica DDDDDanica force-pushed the feature/1852-sentry-feedback branch 6 times, most recently from 32ea0a0 to b30f186 Compare November 7, 2024 01:19
@metamaskbot
Copy link
Collaborator

Builds ready [b30f186]
Page Load Metrics (1793 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16392366179216378
domContentLoaded16322246176914369
load16402358179316177
domInteractive157139147
backgroundConnect7102272311
firstReactRender522861055024
getState45714168
initialActions01000
loadScripts11581681128912661
setupStore1169272210
uiStartup18072635202219292
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 4.74 KiB (0.06%)
  • common: 518 Bytes (0.01%)

itsyoboieltr
itsyoboieltr previously approved these changes Nov 7, 2024
Copy link
Contributor

@itsyoboieltr itsyoboieltr left a comment

Choose a reason for hiding this comment

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

LGTM! The code changes look good to me! I also did the manual testing steps and it all worked for me locally. Great job! Just left one small comment.

app/scripts/lib/setupSentry.js Outdated Show resolved Hide resolved
Copy link
Contributor

@itsyoboieltr itsyoboieltr left a comment

Choose a reason for hiding this comment

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

perfect

@metamaskbot
Copy link
Collaborator

Builds ready [c9d97fe]
Page Load Metrics (1975 ± 100 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint169826071979209100
domContentLoaded16482558194720297
load165626021975209100
domInteractive16184583919
backgroundConnect96125178
firstReactRender462961044723
getState569202110
initialActions01000
loadScripts11912088144618991
setupStore1187352613
uiStartup185128312216243117
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 4.74 KiB (0.06%)
  • common: 524 Bytes (0.01%)

@hjetpoluru
Copy link
Contributor

@DDDDDanica, This is helpful PR for tracking sentry errors.

I notice that when clicking on the Try Again button the application crashes. Could you please check.

Screen.Recording.2024-11-07.at.1.43.21.PM.mov

@DDDDDanica
Copy link
Contributor Author

hey @hjetpoluru thanks for checking, this is expected, as it is not crashing the extension, but close the extension for reload purpose.

@hjetpoluru hjetpoluru self-requested a review November 8, 2024 14:08
Copy link
Contributor

@hjetpoluru hjetpoluru left a comment

Choose a reason for hiding this comment

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

Manually tested and functionality is LGTM and working as expected. Tests are written in POM and documentation is clear. Nice work @DDDDDanica

@DDDDDanica DDDDDanica added this pull request to the merge queue Nov 8, 2024
Merged via the queue into develop with commit 152a50e Nov 8, 2024
76 checks passed
@DDDDDanica DDDDDanica deleted the feature/1852-sentry-feedback branch November 8, 2024 14:36
@github-actions github-actions bot locked and limited conversation to collaborators Nov 8, 2024
@metamaskbot metamaskbot added the release-12.8.0 Issue or pull request that will be included in release 12.8.0 label Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.8.0 Issue or pull request that will be included in release 12.8.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants