fix: Enable wallet push notifications on restart by Prithpal-Sooriya · Pull Request #24752 · MetaMask/metamask-extension

Prithpal-Sooriya · 2024-05-23T18:03:38Z

Description

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

Related issues

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

Manual testing steps

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.

…have enabled notifications.

github-actions · 2024-05-23T18:03:50Z

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.

app/scripts/controllers/metamask-notifications/metamask-notifications.ts

… NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications

metamaskbot · 2024-05-23T20:06:11Z

Builds ready [46bc896]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8
- content-script: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (629 ± 469 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	63	138	86	19	9
		domContentLoaded	9	35	13	6	3
		load	52	2384	629	977	469
		domInteractive	9	35	13	6	3

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 735 Bytes (0.02%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

app/scripts/controllers/push-platform-notifications/services/services.ts

danjm · 2024-05-24T07:27:11Z

app/scripts/controllers/push-platform-notifications/services/services.ts

+): Promise<() => void> {
  // Firebase
  const messaging = await getFirebaseMessaging();
-  const unsubscribe = onBackgroundMessage(


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?

I agree - I will rename this to pushListenerUnsubscribe.

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

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

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.

app/scripts/controllers/metamask-notifications/metamask-notifications.ts

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?

… NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications

Prithpal-Sooriya · 2024-05-28T09:22:01Z

@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 · 2024-05-28T09:58:05Z

Builds ready [35cbb3b]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (667 ± 492 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	179	99	32	15
		domContentLoaded	9	49	14	9	4
		load	53	2560	667	1024	492
		domInteractive	9	49	13	9	4

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 293 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

matteoscurati

Go!

…sing-of-push-notifications

codecov · 2024-05-28T15:10:29Z

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.

metamaskbot · 2024-05-28T15:45:24Z

Builds ready [12e2fb4]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (335 ± 371 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	175	94	24	11
		domContentLoaded	9	19	12	3	1
		load	53	2807	335	772	371
		domInteractive	9	19	12	3	1

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 293 Bytes (0.01%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

Prithpal-Sooriya added 2 commits May 23, 2024 17:57

refactor: tidy up types and push notifications

9a25f28

fix: ensure on startup that users setup push notifications when they …

944a8fd

…have enabled notifications.

Prithpal-Sooriya added the team-notifications-deprecated DEPRECATED: please use "team-assets" instead label May 23, 2024

Prithpal-Sooriya requested a review from a team as a code owner May 23, 2024 18:03

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 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 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 commented May 23, 2024

View reviewed changes

app/scripts/controllers/metamask-notifications/metamask-notifications.ts Show resolved Hide resolved

Prithpal-Sooriya added 2 commits May 23, 2024 20:08

test: test creating push notifications on startup.

fbfdbba

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

46bc896

… NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications

Prithpal-Sooriya requested a review from matteoscurati May 23, 2024 19:08

Prithpal-Sooriya assigned danjm and unassigned danjm May 23, 2024

Prithpal-Sooriya requested a review from danjm May 23, 2024 19:09

matteoscurati previously approved these changes May 23, 2024

View reviewed changes

danjm reviewed May 24, 2024

View reviewed changes

app/scripts/controllers/push-platform-notifications/services/services.ts Show resolved Hide resolved

danjm reviewed May 24, 2024

View reviewed changes

app/scripts/controllers/metamask-notifications/metamask-notifications.ts Show resolved Hide resolved

danjm reviewed May 24, 2024

View reviewed changes

Merge branch 'develop' of github.com:MetaMask/metamask-extension into…

b1877c0

… NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications

refactor: rename variables and docs

35cbb3b

Prithpal-Sooriya dismissed matteoscurati’s stale review via 35cbb3b May 28, 2024 09:24

danjm approved these changes May 28, 2024

View reviewed changes

matteoscurati approved these changes May 28, 2024

View reviewed changes

Merge branch 'develop' into NOTIFY-668-extension-qa-handle-reinitiali…

12e2fb4

…sing-of-push-notifications

Prithpal-Sooriya merged commit 192f9cd into develop May 28, 2024

Prithpal-Sooriya deleted the NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications branch May 28, 2024 15:44

github-actions bot locked and limited conversation to collaborators May 28, 2024

metamaskbot added the release-11.18.0 label May 28, 2024

gauthierpetetin added the release-12.0.0 Issue or pull request that will be included in release 12.0.0 label Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Enable wallet push notifications on restart#24752

fix: Enable wallet push notifications on restart#24752
Prithpal-Sooriya merged 7 commits intodevelopfrom
NOTIFY-668-extension-qa-handle-reinitialising-of-push-notifications

Prithpal-Sooriya commented May 23, 2024 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented May 23, 2024

Uh oh!

Uh oh!

metamaskbot commented May 23, 2024

Uh oh!

Uh oh!

danjm May 24, 2024

Uh oh!

Prithpal-Sooriya May 28, 2024

Uh oh!

Uh oh!

danjm left a comment

Uh oh!

Prithpal-Sooriya commented May 28, 2024 •

edited

Loading

Uh oh!

metamaskbot commented May 28, 2024

Uh oh!

matteoscurati left a comment

Uh oh!

codecov bot commented May 28, 2024

Uh oh!

metamaskbot commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Prithpal-Sooriya commented May 23, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented May 23, 2024

Uh oh!

Uh oh!

metamaskbot commented May 23, 2024

Uh oh!

Uh oh!

danjm May 24, 2024

Choose a reason for hiding this comment

Uh oh!

Prithpal-Sooriya May 28, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danjm left a comment

Choose a reason for hiding this comment

Uh oh!

Prithpal-Sooriya commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented May 28, 2024

Uh oh!

matteoscurati left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 28, 2024

Codecov Report

Uh oh!

metamaskbot commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Prithpal-Sooriya commented May 23, 2024 •

edited by github-actions bot

Loading

Prithpal-Sooriya commented May 28, 2024 •

edited

Loading