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

fix: Enable wallet push notifications on restart #24752

Conversation

Prithpal-Sooriya
Copy link
Contributor

@Prithpal-Sooriya Prithpal-Sooriya commented May 23, 2024

Description

This ensures that users who have enabled wallet notifications will receive push notifications when the application is starting up.

Open in GitHub Codespaces

Related issues

Fixes: Bug when a user stops receiving push notifications if the service worker dies (e.g. browser is closed)

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

After

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

@Prithpal-Sooriya Prithpal-Sooriya added the team-notifications Notifications team label May 23, 2024
@Prithpal-Sooriya Prithpal-Sooriya requested a review from a team as a code owner May 23, 2024 18:03
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.

@Prithpal-Sooriya Prithpal-Sooriya changed the title Notify 668 extension qa handle reinitialising of push notifications fix - enable wallet push notifications on startup for users who have enabled the feature May 23, 2024
@Prithpal-Sooriya Prithpal-Sooriya changed the title fix - enable wallet push notifications on startup for users who have enabled the feature fix: enable wallet push notifications on startup for users who have enabled the feature May 23, 2024
@Prithpal-Sooriya Prithpal-Sooriya changed the title fix: enable wallet push notifications on startup for users who have enabled the feature fix: Enable wallet push notifications on restart May 23, 2024
@Prithpal-Sooriya Prithpal-Sooriya assigned danjm and unassigned danjm May 23, 2024
@Prithpal-Sooriya Prithpal-Sooriya requested a review from danjm May 23, 2024 19:09
@metamaskbot
Copy link
Collaborator

Builds ready [46bc896]
Page Load Metrics (629 ± 469 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint6313886199
domContentLoaded9351363
load522384629977469
domInteractive9351363
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 735 Bytes (0.02%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

matteoscurati
matteoscurati previously approved these changes May 23, 2024
// Firebase
const messaging = await getFirebaseMessaging();
const unsubscribe = onBackgroundMessage(
Copy link
Contributor

Choose a reason for hiding this comment

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

In the file above, there is a rename firebaseUnsubscribe -> pushListenerUnsubscribe. Here there is a rename unsubscribe -> unsubscribeFirebase. What is the difference between these two contexts that lead to the different names?

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 agree - I will rename this to pushListenerUnsubscribe.

In this context, when listening to push notifications we need to listen to 2 things:

  1. When a push notification is received (and how we need to handle it to show to users).
  2. When a push notification is clicked.

Since we have 2 listeners, we need to make sure we unsubscribe from both if they need to be torn down.

Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

Sort of indpendent of this PR, but a question came to me as I was reviewing: can users opt out of push notifications while still enabling the other notification functionality? Should they be able to?

… NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications
@Prithpal-Sooriya
Copy link
Contributor Author

Prithpal-Sooriya commented May 28, 2024

@danjm

Sort of indpendent of this PR, but a question came to me as I was reviewing: can users opt out of push notifications while still enabling the other notification functionality? Should they be able to?

This was discussed somewhere in slack with our PM (see thread) - currently allowing notifications will also allow push notifications. I think we can in the future add more fine grain control (e.g. disable push notifications without disabling notifications).

@metamaskbot
Copy link
Collaborator

Builds ready [35cbb3b]
Page Load Metrics (667 ± 492 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65179993215
domContentLoaded9491494
load5325606671024492
domInteractive9491394
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 293 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@matteoscurati matteoscurati left a comment

Choose a reason for hiding this comment

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

Go!

Copy link

codecov bot commented May 28, 2024

Codecov Report

Attention: Patch coverage is 28.57143% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 65.73%. Comparing base (70b2258) to head (12e2fb4).

Files Patch % Lines
...s/metamask-notifications/metamask-notifications.ts 44.44% 5 Missing ⚠️
...s/push-platform-notifications/services/services.ts 0.00% 4 Missing ⚠️
...tform-notifications/push-platform-notifications.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24752      +/-   ##
===========================================
- Coverage    65.74%   65.73%   -0.00%     
===========================================
  Files         1366     1366              
  Lines        54227    54236       +9     
  Branches     14106    14108       +2     
===========================================
+ Hits         35648    35652       +4     
- Misses       18579    18584       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Prithpal-Sooriya Prithpal-Sooriya merged commit 192f9cd into develop May 28, 2024
70 of 72 checks passed
@Prithpal-Sooriya Prithpal-Sooriya deleted the NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications branch May 28, 2024 15:44
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [12e2fb4]
Page Load Metrics (335 ± 371 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint65175942411
domContentLoaded9191231
load532807335772371
domInteractive9191231
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 293 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@gauthierpetetin gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.0.0 Issue or pull request that will be included in release 12.0.0 team-notifications Notifications team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants