Do notify: Set up base NotificationService #3666

Shadowfiend · 2023-11-09T21:56:46Z

This PR adds notification-related preference, service, optional manifest permission,
and code to request the permission on Subscape banner click.

More to come!

UPDATE: 11/20/23
PR is rebased with the main branch. Force push was required because of unverified commits.
To open a dialog window with a request notification you need to click the link hidden in the Subscape bubble.

The first push notification will be added in #3667

Latest build: extension-builds-3666 (as of Wed, 22 Nov 2023 13:59:40 GMT).

Shadowfiend

Couple of notes from me:

These aren't push notifications (or just push notifications), they're notifications in general, so let's use that term throughout. This is why the original method on the notification service was notify rather than pushNotification, as an example.
Let's decouple the idea of having permission to notify (as defined by manifest v2) from the user having opted in to notifications. Originally this distinction was entirely contained in the PreferencesService. We should not remove the notification permission at the Chrome level once it's been granted, as we don't want to ever present a scenario where the user has to see a browser modal again (we don't control anything about these modals, and in particular we have no ability to give the user more information about why they're granting the permission, so we only present them when we have no other option).
We have both lint failures and unsigned commits here, so let's fix that. We also need to flip out of draft.

Of note, I can't approve (or request changes!) on the PR as I opened it haha. So once we're good here I'll ask @jagodarybacka to merge.

I think if we fix the above we'll probably be good to go.

When setting this to true, the preferences service verifies that the underlying browser permission exists, and refuses to update the setting to true if it does not. This ensures that downstream consumers of the property won't expect to interact with objects that they don't have permissions for.

Missing are updates to test mocks and harnesses at the moment.

This lets us request the permission later on when we need it instead of insta-disabling the extension on upgrade (!!).

This shows the notification request over the Subscape loading page, which may not be the final form of the request.

…ifications

jagodarybacka

A couple of suggestions and questions below.

Could you update the PR description with testing steps? I know we don't have an option to show any notification yet so I wonder what state should I consider as working correctly 😊

background/redux-slices/ui.ts

jagodarybacka · 2023-11-20T13:38:03Z

background/services/preferences/index.ts

+    })
+
+    return permissionRequest.then(async (granted) => {
+      await this.db.setShouldShowNotifications(granted)


User can reject notifications from function setShouldShowNotifications right? So at this point notification permission may not be granted at the browser level but the wallet will think it was granted 🤔 I think we should emit setNotificationsPermission with a result of setShouldShowNotifications instead

It's why we use Promise here: the wallet waits to resolve the promise (true/false) and only then grants him permission, take a look at uiSliceEmitter.on("shouldShowNotifications", ...) in main.ts

User can reject notifications from function setShouldShowNotifications right?

To get more specific here: the above permissionRequest can fail from the user, in which case granted here is false. this.db.setShouldShowNotifications does not trigger a request, however.

background/services/notifications/index.ts

background/redux-slices/ui.ts

ioay · 2023-11-20T17:25:49Z

A couple of suggestions and questions below.

Could you update the PR description with testing steps? I know we don't have an option to show any notification yet so I wonder what state should I consider as working correctly 😊

I've updated today's description in this PR.

jagodarybacka · 2023-11-21T13:16:26Z

background/main.ts


      port.onDisconnect.addListener(() => {
-        const networkNameAtClose =
-          this.store.getState().ui.selectedAccount.network.name
+        const networkNameAtClose = ui.selectedAccount.network.name


is this actually correct? I think UI state will be frozen to the one that we had when this.store.getState() was called - is this expected behaviour? (actually, I'm not sure because I haven't checked if getState gives us a copy of the state object or a state object that gets updated 😅)

We generally should not expect that getState is updated.

For a change like this to an existing behavior, let's make sure we have a commit with a commit message that explains the motivation for the change. A lot of the stuff in main that interacts with redux is very fiddly because it's syncing the lifecycle of the wallet popup with the background script.

This change looks like it can break the desired behavior, which is to check whether the network name changed during the lifetime of the popover. I don't see a clear reason for changing the existing code, and unfortunately the commit message has no details here, so it's hard to know if it's safe without doing a full debugger round.

I have configured Prettier on file saving, so it was changed automatically. It doesn't change logic, so keep it as it is.

The prettier settings we have here by default should not do it 🤔 and I'm pretty sure the logic was changed as we noticed above, right?

omg:)) yees, mea culpa! I didn't notice my change here before! :)

background/services/notifications/index.ts

jagodarybacka · 2023-11-21T16:41:58Z

This is not totally blocking but I think it is a terrible UX 😭

Testing that manually I've found this scenario:

on a fresh install I've added account, clicked on the Subscape link
both wallet connection window and notificatikons prompt were displayed
when I've clicked any option on the notifications prompt:
I couldn't connect the wallet to subscape at all after doing the steps above
at first I haven't noticed but it was happening because I had pending wallet connection window in the background that was lost after I've focused the browser window to confirm/deny notifications

I'm pretty sure this will be a common scenario or our users and that will be a pain to debug and help them because neither dapp nor wallet is indicating where is the problem.

What we can do:

fix the regression on the dapp side of wallet connection blocking the UI rendering
maybe not asking about the notification while asking to connect the wallet at the same time?

Screen.Recording.2023-11-21.at.17.35.21.mov

background/redux-slices/ui.ts

Shadowfiend

All righty let's ship this one. @jagodarybacka I think you have to do the honors 😅

jagodarybacka

✅

@michalinacienciala

## What's Changed * Check connecting to dapp on different network after disconnecting by @michalinacienciala in #3654 * v0.51.0 by @Shadowfiend in #3652 * Fix typos by @xiaolou86 in #3668 * Alchemic Logs: Small improvements for eth_getLogs by @Shadowfiend in #3664 * Fix getting currently connected dapp info by @jagodarybacka in #3671 * Add static subcape link to the wallet by @jagodarybacka in #3670 * Fix e2e tests by @michalinacienciala in #3651 * Add Alchemy endpoint for Arbitrum Sepolia by @jagodarybacka in #3665 * Do notify: Set up base NotificationService by @Shadowfiend in #3666 ## New Contributors * @xiaolou86 made their first contribution in #3668 **Full Changelog**: v0.51.0...v0.52.0 Latest build: [extension-builds-3678](https://github.com/tahowallet/extension/suites/18415208655/artifacts/1067210815) (as of Wed, 22 Nov 2023 14:53:11 GMT).

Shadowfiend force-pushed the alchemic-logs branch 2 times, most recently from 6cc2dfa to 9923b6a Compare November 13, 2023 20:43

Shadowfiend force-pushed the do-notify branch 2 times, most recently from 0bec674 to 74f4de6 Compare November 13, 2023 20:47

Shadowfiend force-pushed the alchemic-logs branch from 9923b6a to af6d8e6 Compare November 13, 2023 21:32

Shadowfiend force-pushed the do-notify branch from 74f4de6 to 39f07d2 Compare November 13, 2023 21:33

Base automatically changed from alchemic-logs to main November 14, 2023 12:23

Shadowfiend commented Nov 18, 2023

View reviewed changes

ioay mentioned this pull request Nov 20, 2023

Push notification draft #3662

Closed

ioay force-pushed the do-notify branch 3 times, most recently from 4c4a57a to 56ee86d Compare November 20, 2023 11:45

Shadowfiend and others added 8 commits November 20, 2023 12:46

Add and set up notifications service

f4b3ad6

Missing are updates to test mocks and harnesses at the moment.

Add optional notifications permission to manifest.json

0737bd1

This lets us request the permission later on when we need it instead of insta-disabling the extension on upgrade (!!).

Request notifications permission on Subscape banner click

6ef0df8

This shows the notification request over the Subscape loading page, which may not be the final form of the request.

Push notifications: added to state & settings

a70b5f7

Settings state connected to background notification service

731486b

Request notification permission on open extension

4d0e740

UI improvements, changes naming conventions: pushNotifications -> not…

f66a2a8

…ifications

ioay force-pushed the do-notify branch from 56ee86d to f66a2a8 Compare November 20, 2023 11:47

ioay marked this pull request as ready for review November 20, 2023 12:00

ioay requested a review from jagodarybacka November 20, 2023 12:00

jagodarybacka reviewed Nov 20, 2023

View reviewed changes

Binding method for event listeners

4d798c5

ioay requested a review from jagodarybacka November 21, 2023 09:49

jagodarybacka reviewed Nov 21, 2023

View reviewed changes

Shadowfiend commented Nov 21, 2023

View reviewed changes

background/redux-slices/ui.ts Outdated Show resolved Hide resolved

Change thunk naming convention, catch the browser issue

554675d

Shadowfiend commented Nov 22, 2023

View reviewed changes

ioay added 2 commits November 22, 2023 10:21

Move protected method to const in the InternalService

f30fe35

reverted getState logic

a7e74f2

jagodarybacka approved these changes Nov 22, 2023

View reviewed changes

Merge branch 'main' into do-notify

9ba58d9

jagodarybacka enabled auto-merge November 22, 2023 13:44

jagodarybacka merged commit 9d01388 into main Nov 22, 2023
6 checks passed

jagodarybacka deleted the do-notify branch November 22, 2023 13:47

jagodarybacka mentioned this pull request Nov 22, 2023

v0.52.0 #3678

Merged

jagodarybacka mentioned this pull request Dec 1, 2023

In wallet Beta notiication tahowallet/dapp#217

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do notify: Set up base NotificationService #3666

Do notify: Set up base NotificationService #3666

Shadowfiend commented Nov 9, 2023 •

edited by github-actions bot

Loading

Shadowfiend left a comment

jagodarybacka left a comment

jagodarybacka Nov 20, 2023

ioay Nov 20, 2023

Shadowfiend Nov 21, 2023

ioay commented Nov 20, 2023

jagodarybacka Nov 21, 2023

Shadowfiend Nov 21, 2023

Shadowfiend Nov 21, 2023

ioay Nov 22, 2023 •

edited

Loading

jagodarybacka Nov 22, 2023

ioay Nov 22, 2023

jagodarybacka commented Nov 21, 2023

Shadowfiend left a comment

jagodarybacka left a comment

Do notify: Set up base NotificationService #3666

Do notify: Set up base NotificationService #3666

Conversation

Shadowfiend commented Nov 9, 2023 • edited by github-actions bot Loading

Shadowfiend left a comment

Choose a reason for hiding this comment

jagodarybacka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioay commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioay Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jagodarybacka commented Nov 21, 2023

Shadowfiend left a comment

Choose a reason for hiding this comment

jagodarybacka left a comment

Choose a reason for hiding this comment

Shadowfiend commented Nov 9, 2023 •

edited by github-actions bot

Loading

ioay Nov 22, 2023 •

edited

Loading