Auth/pm 19877/notification processing #15611
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15611 +/- ##
==========================================
+ Coverage 37.59% 37.83% +0.23%
==========================================
Files 3322 3316 -6
Lines 94923 94081 -842
Branches 14361 14253 -108
==========================================
- Hits 35687 35596 -91
+ Misses 57738 56986 -752
- Partials 1498 1499 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ntation - Initial changeset
…ntation - Minor changes to popup logic and removed content in login component.
…ntation - Trying to have the notification present to prompt the browser extension to popup
…ntation - Giving up on safari. Getting the code ready for review now.
| "build:nonprod:chrome": "cross-env npm run build:chrome", | ||
| "build:nonprod:edge": "cross-env npm run build:edge", | ||
| "build:nonprod:firefox": "cross-env npm run build:firefox", | ||
| "build:nonprod:opera": "cross-env npm run build:opera", | ||
| "build:nonprod:safari": "cross-env npm run build:safari", |
There was a problem hiding this comment.
I added these to prevent the minification of the source code when being built. Is there a better way to do this or is this okay?
There was a problem hiding this comment.
These aren't doing anything special than running their underlying command unless I am missing something?
…ntation - Making final touchups.
…ntation - Removed comment.
…ntation - Cleaned up some unneeded usages.
…ntation - Cleaned up module usage.
…ntation - Cleaned up comments.
…ntation - Added more docs.
…ntation - Added markdown document.
…ntation - Removed unneeded comment.
…tation - Updated condition for if notification is supported.
| switch (this.platformUtilsService.getDevice()) { | ||
| case DeviceType.ChromeExtension: | ||
| chrome.notifications.create(createInfo.id, { | ||
| iconUrl: "https://avatars.githubusercontent.com/u/15990069?s=200", |
There was a problem hiding this comment.
This is a placeholder for a bitwarden icon in the notification. Justin could you provide some guidance on how I could reference an image here for the notification? I imagine referencing an asset would probably be better.
…tation - Updated services module with correct platform utils service.
…tation - Fixed linting bug.
…tation - Renamed server notification service to make more sense.
…tation - Renamed noop server notifications service.
…tation - Fixed bug with not showing body of message.
…tation - Fixed renaming of private member variable notificationsService to serverNotificationsService.
…tation - Fixed typo
…ntation - Removed unnecessary private member variable.
| "build:nonprod:chrome": "cross-env npm run build:chrome", | ||
| "build:nonprod:edge": "cross-env npm run build:edge", | ||
| "build:nonprod:firefox": "cross-env npm run build:firefox", | ||
| "build:nonprod:opera": "cross-env npm run build:opera", | ||
| "build:nonprod:safari": "cross-env npm run build:safari", |
There was a problem hiding this comment.
These aren't doing anything special than running their underlying command unless I am missing something?
| @@ -27,6 +32,15 @@ | |||
| "dist:firefox:mv3": "cross-env MANIFEST_VERSION=3 npm run dist:firefox", | |||
| "dist:opera:mv3": "cross-env MANIFEST_VERSION=3 npm run dist:opera", | |||
| "dist:safari:mv3": "cross-env MANIFEST_VERSION=3 npm run dist:safari", | |||
| "dist:chrome:nonprod": "npm run build:nonprod:chrome && mkdir -p dist && ./scripts/compress.sh dist-chrome.zip", | |||
There was a problem hiding this comment.
What is the use case for doing a non-prod dist build over doing the npm run build:watch:{chrome|firefox|edge|opera|safari} those should have source maps and auto recompile when you make changes.
| @@ -9,7 +9,7 @@ import { | |||
| VaultTimeoutSettingsService, | |||
| VaultTimeoutStringType, | |||
| } from "@bitwarden/common/key-management/vault-timeout"; | |||
| import { NotificationsService } from "@bitwarden/common/platform/notifications"; | |||
| import { ServerNotificationsService } from "@bitwarden/common/platform/notifications"; | |||
There was a problem hiding this comment.
We should change the import location as well.
| import { ServerNotificationsService } from "@bitwarden/common/platform/notifications"; | |
| import { ServerNotificationsService } from "@bitwarden/common/platform/server-notifications"; |
| if ((isChrome || isFirefox) && !isSafari) { | ||
| this.systemNotificationService = new BrowserSystemNotificationService( | ||
| this.logService, | ||
| this.platformUtilsService, | ||
| ); |
There was a problem hiding this comment.
Do you know if a more direct heuristic would work here instead? Like "notifications" in chrome. If ever possible I prefer to not rely on direct client type checks and instead rely on the more core system being requested.
If we can't do that though I don't see the value in having the !isSafari check. the isChrome || isFirefox is enough to know that Safari won't use the supported version.
There was a problem hiding this comment.
There should still be a . before the service in the file name instead of a -.
There was a problem hiding this comment.
Same here with . before service instead of -.
|
|
||
| export type SystemNotificationEvent = { | ||
| id: string; | ||
| type: ButtonActionsKeys; |
There was a problem hiding this comment.
I'm leaning towards not having any type now that I've read the docs a bit more. I think the id can be used well enough and it plays nicer with a recommendation I have about changing the listeners later on.
apps/browser/src/platform/system-notifications/browser-system-notification.service.ts
Show resolved
Hide resolved
| */ | ||
| export abstract class SystemNotificationsService { | ||
| abstract notificationClicked$: Observable<SystemNotificationEvent>; | ||
| abstract create(createInfo: SystemNotificationCreateInfo): Promise<undefined>; |
There was a problem hiding this comment.
Perhaps we should return the notification id back to them that way if they left it off they will get the one that was automatically generated for them?
| }; | ||
|
|
||
| export type SystemNotificationCreateInfo = { | ||
| id: string; |
There was a problem hiding this comment.
This can be optional right?
…tation - Fixed console log.
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19877
📔 Objective
These changes lay the foundation and support it in the platform domain to do operating system notifications.
isViewOpento clearerisPopupOpenfor a clearer api.📸 Screenshots
Open Popup
The actions service working in Chrome. Code that demonstrated this was removed.
Chrome.Working.mov
System Notifications
The notifications service working in Chrome. Code that demonstrated this was removed.
Chrome.Notifications.Working.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes